Refactor into distinct packages to prep for adding firewall support to Nodes #186
Conversation
AshleyDumaine
force-pushed
the
fw-nodes
branch
2 times, most recently
from
edc6484 to
d897018
Compare
cloud/linode/client/client.go
Outdated
| @@ -39,7 +39,7 @@ type Client interface { | |||
| // linodego.Client implements Client | |||
| var _ Client = (*linodego.Client)(nil) | |||
|
|
|||
| func newLinodeClient(token, ua, apiURL string) (*linodego.Client, error) { | |||
| func NewLinodeClient(token, ua, apiURL string) (*linodego.Client, error) { | |||
There was a problem hiding this comment.
Since this function is now exported, I would recommend adding some godoc.
This is a bit of a nit, but client.NewLinodeClient kind of stutters. Maybe calling the function New would flow a bit more nicely.
Lastly, please expand the ua argument to userAgent, so that even without godoc, the argument's purpose is clear.
| @@ -1,6 +1,6 @@ | |||
| package linode | |||
| package client | |||
There was a problem hiding this comment.
Just a heads up: moving this file into a new package would constitute a major version bump of this module. However, this module's last-tagged version is still < v1, so it should be fine. Just be aware that this may break downstream users of this module.
cloud/linode/firewall/firewalls.go
Outdated
| var newFirewallID int | ||
| var err error | ||
|
|
||
| fwID := service.GetAnnotations()[annotations.AnnLinodeCloudFirewallID] | ||
| newFirewallID, err = strconv.Atoi(fwID) |
There was a problem hiding this comment.
nit: There is no need to declare newFirewallID and err this high up.
| var newFirewallID int | |
| var err error | |
| fwID := service.GetAnnotations()[annotations.AnnLinodeCloudFirewallID] | |
| newFirewallID, err = strconv.Atoi(fwID) | |
| fwID := service.GetAnnotations()[annotations.AnnLinodeCloudFirewallID] | |
| newFirewallID, err := strconv.Atoi(fwID) |
cloud/linode/firewall/firewalls.go
Outdated
| err = l.Client.DeleteFirewallDevice(ctx, existingFirewallID, deviceID) | ||
| if err != nil { |
There was a problem hiding this comment.
nit:
| err = l.Client.DeleteFirewallDevice(ctx, existingFirewallID, deviceID) | |
| if err != nil { | |
| if err := l.Client.DeleteFirewallDevice(ctx, existingFirewallID, deviceID); err != nil { |
There was a problem hiding this comment.
updated and in a few other spots in this file too
General:
Pull Request Guidelines:
This PR moves out the Firewall logic into its own
firewallpackage so it can be used for managing Firewalls for both LoadBalancer Services (NodeBalancers) and Nodes.I also split out the annotations and client since I was getting an import cycle with just the existing
linodeand newfirewallpackages.The change to add support for Firewalls based on Node annotations will be tackled in a separate PR.