-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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.
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
Allow ENTERPRISE_PLUS as default Edition for POSTGRES_16 #12077
Allow ENTERPRISE_PLUS as default Edition for POSTGRES_16 #12077
Conversation
Hello! I am a robot. Tests will require approval from a repository maintainer to run. @rileykarson, a repository maintainer, has been assigned to review your changes. If you have not received review feedback within 2 business days, please leave a comment on this PR asking them to take a look. You can help make sure that review is quick by doing a self-review and by running impacted tests locally. |
@@ -2090,7 +2089,7 @@ func flattenSettings(settings *sqladmin.Settings, d *schema.ResourceData) []map[ | |||
data := map[string]interface{}{ | |||
"version": settings.SettingsVersion, | |||
"tier": settings.Tier, | |||
"edition": flattenEdition(settings.Edition), | |||
"edition": settings.Edition, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will a null
response (including complete omission) still always imply ENTERPRISE
? That's generally how these kind of cases work (where there was a fixed default that starts to vary on the API side). We should preserve the flattening logic if so, that'll keep the values recorded in state consistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The CloudSQL API actually uses default Edition as EDITION_UNSPECIFIED
(refer: type Settings) when the customer doesn't specify the edition. Today a EDITION_UNSPECIFIED
instance is configured similar to a ENTERPRISE
instance but this could change within the CloudSQL API which will create inconsistent behavior for the customers when dealing directly with CloudSQL API vs Terraform provider.
I agree that flattening the value keeps the recorded states consistent, but it sort of goes against what we have documented in this regard (as pointed above). But if preserving the flattening logic makes this change safer than I agree that we should keep it.
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
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. |
Tests analyticsTotal tests: 101 Click here to see the affected service packages
Action takenFound 79 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
Override reason: Default -> Computed can change the value received by end user configs, but this matches new API behaviour and the API behaviour is guarded on new product-internal versions (SQL database versions here). We've discussed this as acceptable offline, although would consider batching in a major release if we were closer to one. |
🟢 Tests passed during RECORDING mode: 🟢 No issues found for passed tests after REPLAYING rerun. 🔴 Tests failed during RECORDING mode: 🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR. |
Test failures are due to missing business logic, and the fix is obvious given how the product behaves. I'll get that merged as #12096, any push of yours after that happens will integrate that change and tests will move forward. |
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
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. |
Tests analyticsTotal tests: 101 Click here to see the affected service packages
Action takenFound 3 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
🟢 Tests passed during RECORDING mode: 🟢 No issues found for passed tests after REPLAYING rerun. 🔴 Tests failed during RECORDING mode: 🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR. |
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
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. |
Tests analyticsTotal tests: 101 Click here to see the affected service packages
Action takenFound 23 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
🟢 Tests passed during RECORDING mode: 🟢 No issues found for passed tests after REPLAYING rerun. 🔴 Tests failed during RECORDING mode: 🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR. |
TestAccSqlDatabaseInstance_Edition_Downgrade is failing because the test was changed in this PR, but would have failed before this change (and before #12096) if the test was written as it now is, and does represent a fiddly interaction. I'll follow up tomorrow to determine how we want to handle it. I don't think that this PR or #12096 needs to block on a solution. Specifically when the edition is downgraded, the data cache feature must simultaneously not appear in the request / be disabled.
Given that downgrades are an unlikely flow and the field had a permadiff before, I think that's acceptable to move forward with and just fix-forward in a later release (or cherrypick a fix sooner if this assessment is wrong). |
…Platform#12077) Co-authored-by: Sarthak Tandon <[email protected]>
…Platform#12077) Co-authored-by: Sarthak Tandon <[email protected]>
…Platform#12077) Co-authored-by: Sarthak Tandon <[email protected]>
…Platform#12077) Co-authored-by: Sarthak Tandon <[email protected]>
…Platform#12077) Co-authored-by: Sarthak Tandon <[email protected]>
Current default:
ENTERPRISE
forsettings.edition
is not consistent with the API which treatsEDITION_UNSPECIFIED
as the default (refer link) when customer doesn't provide any. With PostgreSQL version 16, the default Edition will beENTERPRISE_PLUS
as documented in the PostgreSQL June 07, 2024 release notes. Thus, unset the defaultedition
so that it can instead be computed in the API, allowing the API to useENTERPRISE_PLUS
as the default for POSTGRES_16.b/338258663
I acknowledge that I have:
make test
andmake lint
in the generated providers to ensure it passes unit and linter tests.Release Note Template for Downstream PRs (will be copied)