Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(products): Product-specific enablement and configuration. #570

Merged
merged 8 commits into from
Dec 17, 2024
Merged

feat(products): Product-specific enablement and configuration. #570

merged 8 commits into from
Dec 17, 2024

Conversation

kpfleming
Copy link
Contributor

@kpfleming kpfleming commented Dec 10, 2024 •
edited
Loading

Product-specific enablement and configuration operations have been added under the 'fastly/products/ package path in this module. These are intended to replace the non-specific 'GetProduct', 'EnableProduct', and 'DisableProduct' operations, which have been marked as deprecated.

By adding these operations, support for products which require configuration during enablement (Next-Gen WAF), and/or support configuration after enablement (Next-Gen WAF and DDoS Protection) is now available. The operations for those products have product-specific Input and Output structures which carry the additional attributes supported by the products.

In addition, the functional tests for all product enablement operations have been enhanced to more thoroughly test the operations.

Review strategy recommendations:

  1. Ignore the YAML files; they are all test fixtures generated by running the tests, and the tests pass when run against api.fastly.com. (Tip: Use the 'File Filter' in the PR review interface to exclude them).
  2. Start with fastly/products to see how product APIs and their tests are defined. Most of the files in the packages are copies of each other, except for ngwaf and ddos_protection which contain additional behavior and tests.
  3. Go to internal/productcore to see how the generic API operations and functional-test constructors are implemented.
  4. Finally, review fastly_test_utils.go to see how table-driven functional tests are executed (this should be used in the future to replace the existing functional test mechanisms throughout the source tree).

@kpfleming kpfleming marked this pull request as ready for review December 10, 2024 16:30
@kpfleming
Copy link
Contributor Author

This also adds a dependency on the testify module, but it is very useful and we're already using it in terraform-provider-fastly.

philippschulte
philippschulte approved these changes Dec 12, 2024
View reviewed changes
Copy link
Member

@philippschulte philippschulte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the code looks great! However, I have never used go generics so it might be good to ask for more reviews. Thanks for the work!

internal/products/api_base.go Outdated Show resolved Hide resolved
@kpfleming
feat(products): Product-specific enablement and configuration.
4abff11 
Product-specific enablement and configuration operations have been
added under the 'fastly/products/ package path in this module. These
are intended to replace the non-specific 'GetProduct',
'EnableProduct', and 'DisableProduct' operations, which have been
marked as deprecated.

By adding these operations, support for products which require
configuration during enablement (Next-Gen WAF), and/or support
configuration after enablement (Next-Gen WAF and DDoS Protection) is
now available. The operations for those products have product-specific
Input and Output structures which carry the additional attributes
supported by the products.

In addition, the functional tests for all product enablement
operations have been enhanced to more thoroughly test the operations.
@kpfleming
Move image_optimizer_default_settings_test to its own package to avoid
a074718 
an import cycle.
@kpfleming
ConfigureOutputNested struct does not need to be exported.
23e9a6e
@kpfleming
Apply review feedback.
de962ad
jsocol
jsocol approved these changes Dec 12, 2024
View reviewed changes
fastly/fastly_test_utils.go Outdated Show resolved Hide resolved
internal/products/functional_test_enable.go Outdated Show resolved Hide resolved
cee-dub
cee-dub reviewed Dec 12, 2024
View reviewed changes
fastly/products/bot_management/api.go Outdated Show resolved Hide resolved
fastly/products/ddos_protection/api_test.go Outdated Show resolved Hide resolved
internal/products/api_put.go Outdated Show resolved Hide resolved
internal/products/api_get.go Outdated Show resolved Hide resolved
internal/products/types.go Outdated Show resolved Hide resolved
@kpfleming
Apply some review feedback: use ProductOutput constraint everywhere.
154d5de
@kpfleming
Apply review feedback:
449ec7b 
1. Rename internal/products to internal/productcore

2. Eliminate import aliases

3. Eliminate type parameters on structs that don't use them

4. Use pointer receivers for methods in ProductOutput interface
@kpfleming
Copy link
Contributor Author

Applied review feedback:

  1. Renamed internal/products to internal/productcore

  2. Eliminated import aliases

  3. Eliminated type parameters on structs that don't use them

  4. Used pointer receivers for methods in ProductOutput interface

The elimination of the import aliases has made the code... quite verbose.

@cee-dub
Copy link
Contributor

cee-dub commented Dec 13, 2024

I never meant to take a hard line on removing all import aliases.

@kpfleming
Copy link
Contributor Author

I never meant to take a hard line on removing all import aliases.

I didn't interpret your comments that way, but once the mention of renaming internal/products was there, it made sense to me. Also, copy-pasting to make new products is not going to happen often enough that it needs to be made easier by using aliases :-)

@kpfleming
More review feedback:
4cc01eb 
Reverse order of input and output type parameters, which allows one of
them to be inferred.
@kpfleming
Copy link
Contributor Author

Reversing the order of the type parameters to follow Go practices allowed one of them to be inferred (as @cee-dub had demonstrated), which reduced the verbosity. Definitely the right change.

@kpfleming
More review feedback:
6144a31 
Move new functional-testing support into internal/test_utils package.
cee-dub
cee-dub approved these changes Dec 16, 2024
View reviewed changes
Copy link
Contributor

@cee-dub cee-dub left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only feedback here is that Go package names typically avoid using underscores, so it would be testutils. I'm not going to block you on that!

@kpfleming kpfleming merged commit 824cab9 into fastly:main Dec 17, 2024
3 checks passed
@kpfleming kpfleming deleted the cdtool-966 branch December 17, 2024 11:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@philippschulte philippschulte philippschulte approved these changes

@jsocol jsocol jsocol approved these changes

@cee-dub cee-dub cee-dub approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kpfleming @cee-dub @jsocol @philippschulte