resource/cloudflare_ruleset: fix various attributes #2511
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
changelog detected ✅ |
zakcutner
force-pushed
the
fix-ruleset-resource
branch
from
26b2e0a to
b5e8c93
Compare
zakcutner
changed the title
jacobbednarz
reviewed
Jun 9, 2023
|
we'll also need to add the newly computed fields to https://github.com/zakcutner/terraform-provider-cloudflare/blob/0f6d5c7d0611c27116fbc0fee96cc3388d76c669/internal/framework/service/rulesets/resource.go#L1341 so that they aren't a part of the hash. |
cjolowicz
reviewed
Jun 9, 2023
These attributes are solely computed by the Rulesets API and so should never be set by users of the API. Having these as `Optional` attributes implied that users had control over them. This was particularly confusing because they were being populated by `cf-terraforming`, since they were marked as `Optional`. Also populate the last updated attribute from API responses.
If users do not set this attribute, it is computed by the Ruleset API (it will be set to the same value as the rule's ID).
Add the `stringplanmodifier.RequiresReplace()` modifier since this attribute cannot be changed without re-creating the ruleset. Also populate this attribute in API requests and from API responses.
zakcutner
force-pushed
the
fix-ruleset-resource
branch
from
0f6d5c7 to
5e93bae
Compare
Thanks for pointing me to this! I've updated the way it works a little in 1b4d844. The important change (in addition to the one you mentioned) is to ensure that setting the ref manually causes you to be "opted out" of this remapping. |
|
Also, I wanted to ask if you would recommend adding a state migration for the breaking change to make the |
zakcutner
force-pushed
the
fix-ruleset-resource
branch
from
5e93bae to
34c2d5b
Compare
This is safe because all rules have a ref, as it defaults to the ID if it is not explicitly set. The advantage of using refs is that we can take into account whether the user has explicitly set the ref themselves. If they have, then we avoid changing it, or using that ref for other rules.
Use the `skip` action, rather than the `allow` action. The `allow` action does not exist in Rulesets, and only existed in Firewall Rules. Make the Origin Rules tests agnostic of the zone they are run with. Add `(cf.zone.plan eq "ENT")` to the expression of account-level WAF rulesets, as this is a new requirement. Also add the `stringplanmodifier.UseStateForUnknown()` modifier to the description attribute for rules, since this should never change when it is computed.
zakcutner
force-pushed
the
fix-ruleset-resource
branch
from
34c2d5b to
603399f
Compare
as we discussed, i don't think we need this given the hoops folks would have had to jump through to get |
|
acceptance tests all looking good |
|
This functionality has been released in v4.8.0 of the Terraform Cloudflare Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you! |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Make some minor fixes to the Rulesets acceptance resource tests
Use the
skipaction, rather than theallowaction. Theallowaction does not exist in Rulesets, and only existed in Firewall Rules.
Make the Origin Rules tests agnostic of the zone they are run with.
Add
(cf.zone.plan eq "ENT")to the expression of account-level WAFrulesets, as this is a new requirement.
Also add the
stringplanmodifier.UseStateForUnknown()modifier to thedescription attribute for rules, since this should never change when
it is computed.
Update preservation of rules to use refs rather than IDs
This is safe because all rules have a ref, as it defaults to the ID if
it is not explicitly set. The advantage of using refs is that we can
take into account whether the user has explicitly set the ref
themselves. If they have, then we avoid changing it, or using that ref
for other rules.
Fix ruleset rule shareable entitlement name attribute
Add the
stringplanmodifier.RequiresReplace()modifier since thisattribute cannot be changed without re-creating the ruleset.
Also populate this attribute in API requests and from API responses.
Mark ruleset rule ref attribute as
ComputedIf users do not set this attribute, it is computed by the Ruleset API
(it will be set to the same value as the rule's ID).
Prevent ruleset rule ID, version and last updated attributes being set
These attributes are solely computed by the Rulesets API and so should
never be set by users of the API. Having these as
Optionalattributesimplied that users had control over them.
This was particularly confusing because they were being populated by
cf-terraforming, since they were marked asOptional.Also populate the last updated attribute from API responses.
Fix typo in
semaphoreErrvariable name