feat(product): Add 'product' commands for each Fastly product. #1362
Conversation
Adds 'enable', 'disable', and 'status' commands for each product previously supported by the 'products' command (which is now deprecated). Also adds 'configure' commands for products which support configuration (DDoS Protection and Next-Gen WAF).
kpfleming
added
DO NOT MERGE YET
dependencies
| _, err = GetFn(c.Globals.APIClient, serviceID) | ||
| if err != nil { | ||
| var herr *fastly.HTTPError | ||
|
|
||
| // The API returns a 'Bad Request' error when the | ||
| // product has not been enabled on the service; any | ||
| // other error should be reported | ||
| if !errors.As(err, &herr) || !herr.IsBadRequest() { | ||
| c.Globals.ErrLog.Add(err) | ||
| return err | ||
| } | ||
| } else { | ||
| s.Enabled = true | ||
| } | ||
|
|
||
| if ok, err := c.WriteJSON(out, s); ok { | ||
| return err | ||
| } | ||
|
|
||
| var msg string | ||
| if s.Enabled { | ||
| msg = "enabled" | ||
| } else { | ||
| msg = "disabled" | ||
| } | ||
|
|
||
| text.Info(out, | ||
| bot_management.ProductName+" is %s on service %s", msg, serviceID) | ||
|
|
||
| return nil |
There was a problem hiding this comment.
This logic is a bit hard to follow. One idea...
| _, err = GetFn(c.Globals.APIClient, serviceID) | |
| if err != nil { | |
| var herr *fastly.HTTPError | |
| // The API returns a 'Bad Request' error when the | |
| // product has not been enabled on the service; any | |
| // other error should be reported | |
| if !errors.As(err, &herr) || !herr.IsBadRequest() { | |
| c.Globals.ErrLog.Add(err) | |
| return err | |
| } | |
| } else { | |
| s.Enabled = true | |
| } | |
| if ok, err := c.WriteJSON(out, s); ok { | |
| return err | |
| } | |
| var msg string | |
| if s.Enabled { | |
| msg = "enabled" | |
| } else { | |
| msg = "disabled" | |
| } | |
| text.Info(out, | |
| bot_management.ProductName+" is %s on service %s", msg, serviceID) | |
| return nil | |
| state := "disabled" | |
| _, err = GetFn(c.Globals.APIClient, serviceID) | |
| if err != nil { | |
| // The API returns a 'Bad Request' error when the product has | |
| // not been enabled on the service so we ignore it; any other | |
| // error should be reported | |
| var herr *fastly.HTTPError | |
| if !errors.As(err, &herr) || !herr.IsBadRequest() { | |
| c.Globals.ErrLog.Add(err) | |
| return err | |
| } | |
| } else { | |
| s.Enabled = true | |
| state = "enabled" | |
| } | |
| if ok, err := c.WriteJSON(out, s); ok { | |
| return err | |
| } | |
| text.Info(out, bot_management.ProductName+" is %s on service %s", state, serviceID) | |
| return nil |
There was a problem hiding this comment.
Ahh, yes, that's definitely better. I'll roll this into some other changes in the branch.
There was a problem hiding this comment.
Sorry, I didn't use my normal editor and looks like I have a syntax error in there.
| @@ -0,0 +1,112 @@ | |||
| package bot_management | |||
There was a problem hiding this comment.
The linter is agreeing with me that package names shouldn't have underscores.
There was a problem hiding this comment.
Yeah... I've been trying to stick with using our actual product IDs for ease of understanding by future maintainers, but the Go community has a different opinion (except for test packages, where underscores are allowed).
There was a problem hiding this comment.
In that case it is representing a build tag. The same works for other specific words that match OS targets or architectures, like _linux or _arm64.
There was a problem hiding this comment.
There's also the only exception to two packages existing in one directory, where p and p_test can coexist to allow for blackbox testing of p.
| Globals: g, | ||
| }, | ||
| } | ||
| c.CmdClause = parent.Command("disable", "Disable the "+bot_management.ProductName+" product") |
There was a problem hiding this comment.
At first, I was rather confused until I realized that bot_management.ProductName refers to github.com/fastly/go-fastly/v9/fastly/products/bot_management.ProductName and not to something in this package. Same for the rest of the bot_management.Things in pkg/commands/product/bot_management/*.go files.
There was a problem hiding this comment.
I was confused by that when I started working in Go; if you're in package A, and you've also imported a package named A, then qualified references like A.foo always refer to the imported package and never to the 'local' package. Now that I've gotten used to it I'm not bothered by it, but this sort of thing is what prompted the desire for import aliases in the other project :-)
There was a problem hiding this comment.
Might I suggest an alias is probably appropriate here? bot or botapi could work
There was a problem hiding this comment.
This is what got me down the road of using a 'generic' import alias in the other project; if there are 9 packages that all import their corresponding 'API' functionality from another module, they could use the same import alias, to keep things consistent for the reader.
What crosses my mind is that if I see bot.ProductName and ddos.ProductName I might think that those are different in some way, but if they are both api.ProductName I'll know right away that they represent the same concept. Granted, calling it <anything>.ProductName should also be sufficient to express the concept...
There was a problem hiding this comment.
Try it and see what feels best, I guess!
|
|
||
| // DisableFn is a dependency-injection point for unit tests to provide | ||
| // a mock implementation of the API operation. | ||
| var DisableFn = func(client api.Interface, serviceID string) error { |
There was a problem hiding this comment.
Probably a Go idiom or just a style preference of my own from familiarity with the standard library, but i suggest DisableFunc and friends.
Adds 'enable', 'disable', and 'status' commands for each product previously supported by the 'products' command (which is now deprecated).
Also adds 'configure' commands for products which support configuration (DDoS Protection and Next-Gen WAF).