add support for universal deletion_policy#17380
add support for universal deletion_policy#17380NickElliot wants to merge 111 commits intoGoogleCloudPlatform:mainfrom
deletion_policy#17380Conversation
|
Hi there, I'm the Modular magician. I've detected the following information about your changes for commit 4faf853: Diff reportYour PR generated the following diffs in downstream repositories:
Breaking Change(s) DetectedThe following breaking change(s) were detected within your pull request.
If you believe this detection to be incorrect please raise the concern with your reviewer. Missing doc report (experimental)The following resources have fields missing in documents.
The following data sources are missing documents:
Test reportAnalytics
Affected Service Packages
Step 1: Replaying Mode Action takenFound 15 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
View the build log Step 2: Recording Mode
Caution Issues requiring attention before PR completion 🔴 Initial Recording Failed: Some tests failed during the recording step. See the table above for details. Please address these issues to complete your PR. If you believe these detections are incorrect or unrelated to your change, please raise the concern with your reviewer. View the build log or the debug logs folder for detailed results. @NickElliot VCR tests complete for 4faf853! |
|
all standard test failures unrelated to deletion policy bringing in @zli82016 as a second reviewer for the TGC tests issues/for extra eyes given the size of the PR |
There was a problem hiding this comment.
Can you help me out by re-explaining some important points in a singular place here, or in the main PR comment? It's been a long process of reviewing individual PRs within the feature branch, and each of those were focused pieces. We should take a step back and double check we've covered everything here at the end, and it's hard to remember all the pieces to track and decisions we've made a long the way.
If this PR were to break things and someone that wasn't you or I were to try to understand the changes here, it'd be too big to decipher easily. A summary of the high-level changes in this PR would be really helpful.
My main questions right now:
- are the findings from the breaking change detector expected? From what I understood we were preserving the defaults via
deletion_policy_default. If we are ignoring the breaking change detector, how do we know we haven't accidentally swapped a default? - Is there a follow up PR expected for the provider-scoped UDP documentation?
- Are we planning on having contribution documentation for UDP?
- a few todo/to-do's in this final merge, are those tracked somewhere, or should they be addressed now?
|
will write something up! |
|
The failed tgc integration tests are unrelated and have been fixed in the main branch. |
I updated the top comment with full details.
|
|
Hi there, I'm the Modular magician. I've detected the following information about your changes for commit eb0746d: Diff reportYour PR generated the following diffs in downstream repositories:
Breaking Change(s) DetectedThe following breaking change(s) were detected within your pull request.
If you believe this detection to be incorrect please raise the concern with your reviewer. Missing doc report (experimental)The following resources have fields missing in documents.
The following data sources are missing documents:
Test reportAnalytics
Affected Service Packages
Step 1: Replaying Mode Action takenFound 19 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
View the build log Step 2: Recording Mode
Caution Issues requiring attention before PR completion 🔴 Initial Recording Failed: Some tests failed during the recording step. See the table above for details. 🔴 Replaying Rerun Failed: Some tests failed due to non-determinism when VCR replayed the response. See the table above for details. Please address these issues to complete your PR. If you believe these detections are incorrect or unrelated to your change, please raise the concern with your reviewer. View the build log or the debug logs folder for detailed results. @NickElliot, @zli82016 VCR tests complete for eb0746d! |
|
Hi there, I'm the Modular magician. I've detected the following information about your changes for commit d7c9d31: Diff reportYour PR generated the following diffs in downstream repositories:
Breaking Change(s) DetectedThe following breaking change(s) were detected within your pull request.
If you believe this detection to be incorrect please raise the concern with your reviewer. Missing doc report (experimental)The following resources have fields missing in documents.
The following data sources are missing documents:
Test reportAnalytics
Affected Service Packages
Step 1: Replaying Mode Action takenFound 21 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
View the build log Step 2: Recording Mode
Caution Issues requiring attention before PR completion 🔴 Initial Recording Failed: Some tests failed during the recording step. See the table above for details. 🔴 Replaying Rerun Failed: Some tests failed due to non-determinism when VCR replayed the response. See the table above for details. Please address these issues to complete your PR. If you believe these detections are incorrect or unrelated to your change, please raise the concern with your reviewer. View the build log or the debug logs folder for detailed results. @NickElliot, @zli82016 VCR tests complete for d7c9d31! |
c2thorn
left a comment
There was a problem hiding this comment.
I believe this is good to go, the contributor docs can go any time after.
Verified the defaults match their previous values, so I believe we are good there.
Thank you for the write up and explanations.
zli82016
left a comment
There was a problem hiding this comment.
LGTM except that the unit test failed.
|
Hi there, I'm the Modular magician. I've detected the following information about your changes for commit 8a5493d: Diff reportYour PR generated the following diffs in downstream repositories:
Breaking Change(s) DetectedThe following breaking change(s) were detected within your pull request.
If you believe this detection to be incorrect please raise the concern with your reviewer. Missing doc report (experimental)The following resources have fields missing in documents.
The following data sources are missing documents:
Test reportAnalytics
Affected Service Packages
Step 1: Replaying Mode Action takenFound 16 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
View the build log Step 2: Recording Mode
Caution Issues requiring attention before PR completion 🔴 Initial Recording Failed: Some tests failed during the recording step. See the table above for details. 🔴 Replaying Rerun Failed: Some tests failed due to non-determinism when VCR replayed the response. See the table above for details. Please address these issues to complete your PR. If you believe these detections are incorrect or unrelated to your change, please raise the concern with your reviewer. View the build log or the debug logs folder for detailed results. @NickElliot VCR tests complete for 8a5493d! |
|
Hi there, I'm the Modular magician. I've detected the following information about your changes for commit ee0994e: Diff reportYour PR generated the following diffs in downstream repositories:
Breaking Change(s) DetectedThe following breaking change(s) were detected within your pull request.
If you believe this detection to be incorrect please raise the concern with your reviewer. Missing doc report (experimental)The following resources have fields missing in documents.
The following data sources are missing documents:
Test reportAnalytics
Affected Service Packages
Step 1: Replaying Mode Action takenFound 17 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
View the build log Step 2: Recording Mode
Caution Issues requiring attention before PR completion 🔴 Initial Recording Failed: Some tests failed during the recording step. See the table above for details. 🔴 Replaying Rerun Failed: Some tests failed due to non-determinism when VCR replayed the response. See the table above for details. Please address these issues to complete your PR. If you believe these detections are incorrect or unrelated to your change, please raise the concern with your reviewer. View the build log or the debug logs folder for detailed results. @NickElliot VCR tests complete for ee0994e! |
|
Hi there, I'm the Modular magician. I've detected the following information about your changes for commit 365c06f: Diff reportYour PR generated the following diffs in downstream repositories:
Breaking Change(s) DetectedThe following breaking change(s) were detected within your pull request.
If you believe this detection to be incorrect please raise the concern with your reviewer. Missing doc report (experimental)The following resources have fields missing in documents.
The following data sources are missing documents:
|
Universal deletion_policy is a string field added to the top-level schema of all compatible resources. Rather than using a “default” attribute, it is configured as an optional+computed field.
To display a default from provider configuration or resource settings, it is dependent on a new customize diff function that operates in a similar way to the customize diff functions for displaying Project/Region/Zone at plan-time. A GetRawConfig() check is performed on the resource to verify if it has been set explicitly by the user, and in absence of that checks for if a user has configured a value within their provider configuration, and if that is absent it goes with the resource default. There is no fall back state behind this, as all resources have a default supplied to this customize diff function as a string argument.
At the end of the read function some code has been included for setting virtual fields to their default (following standard design for TPG resources), with the minor addition of checking the provider configuration before using the resource default.
If a resource has been excluded from supporting resourceUpdate() previously, it has been updated to have an Update function that immediately returns resourceRead(). Existing update functions have had a PreUpdate check introduced at the beginning, which quickly checks over the resource to verify if any updates have been planned other than those to deletion-policy, and if only deletion_policy is being updated, will short circuit the function into returning resource*Read().
Lastly, a PreDelete check has been added that performs the logic for either “PREVENT” or “ABANDON”. If necessary due to some implementations using different terms (e.g. “DEFAULT” often being used in place of “DELETE” previously), existing implementations have been updated to be compatible with these standard terms.
Supporting the above implementation, three attributes have been added to resource.go for usage in resource yaml files:
deletion_policy_exclude, if set to true this will avoid generating the deletion_policy related fields to the end resource. This is set to true for resources that are incompatible with a deletion policy (such as those with no delete function), or those that had existing, incompatible implementations.
deletion_policy_custom_docs, if set to true this will avoid generating the standard docs description for the universal deletion policy field. This is set to true for resources which support custom options in their deletion_policy such as “force”, and need a bespoke description for their usage. These resources have maintained their virtual field deletion_policy fields, but updated to have an “exclude: true” attribute that will avoid them being generated into the downstream resource (as these resources will use the fully standard field/associated code in most areas, with their custom code generally being contained within a pre_delete constant.
deletion_policy_default, a string attribute that can be set to the desired default value for a resource. This will default to “DELETE” if unset.
One attribute deletion_policy has been added to provider.go and config.go, and is used to enable configuring the provider-level default.
Release Note Template for Downstream PRs (will be copied)
See Write release notes for guidance.