[CDTOOL-1187] Add support for the 'mode' parameter when enabling DDoS Protection #796
[CDTOOL-1187] Add support for the 'mode' parameter when enabling DDoS Protection #796
Conversation
…bling DDOS Protection
philippschulte
left a comment
There was a problem hiding this comment.
This looks functionally correct, and the added fixtures/tests for explicit mode are good.
My only concern is backward compatibility: changing Enable(...) to require an EnableInput argument is a breaking API change for existing consumers!
| // Enable enables the DDoS Protection product on the service. | ||
| func Enable(ctx context.Context, c *fastly.Client, serviceID string) (EnableOutput, error) { | ||
| return productcore.Put[EnableOutput](ctx, &productcore.PutInput[products.NullInput]{ | ||
| func Enable(ctx context.Context, c *fastly.Client, serviceID string, i EnableInput) (EnableOutput, error) { |
There was a problem hiding this comment.
This is a breaking change for all existing callers, even if they do not care about mode!
In Go there is no function overloading, so current code will stop compiling after upgrading.
There was a problem hiding this comment.
Hmm, good point. I instead created a new function in 969cd5d which allows a custom input. Enable now still acts the same by relying on the API default for the 'Mode' param - making this a non-breaking change. I have added comments so users are aware of this.
Let me know if this pattern is acceptable.
Reverted the breaking change to the 'Enable' func by maintaining the same "default" mode behavior and creating a new 'EnableWithInput' func which can be used to specify a 'mode'.
|
Given the very low number of users of go-fastly, I think a breaking change would be fine, and preferable to having a separate function. For the future, we should probably standardize on all 'action' functions accepting an input structure even if it is empty, so we can add fields to that structure in the future without a breaking change. |
philippschulte
left a comment
There was a problem hiding this comment.
Looks good to me now. Preserving Enable(...) and adding EnableWithInput(...) avoids the breaking API change while still exposing mode. The added tests for explicit mode and verification are great. Thank you!
Change summary
This PR adds support for passing the
modeparameter when calling theEnableDDOS Protection endpoint. This was previously missing and only available throughUpdate.All Submissions:
New Feature Submissions:
Changes to Core Features: