-
Notifications
You must be signed in to change notification settings - Fork 225
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
Add SQLInstance network mockgcp tests #2563
Add SQLInstance network mockgcp tests #2563
Conversation
...urcefixture/testdata/basic/sql/v1beta1/sqlinstance/sqlinstanceauthorizednetworks/create.yaml
Outdated
Show resolved
Hide resolved
(with terraform-based controller)
(with direct controller enabled)
3ab9221
to
c95b85f
Compare
c95b85f
to
f77add2
Compare
} | ||
|
||
// Note: For some reason, the KRM API allows []InstancePscConfig. However, in the GCP proto there is only | ||
// a single *api.PscConfig. I think there is a mistake in the KRM API; it should not allow a list. For |
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.
This seems likely, there is a lot of ambiguity in the Terraform nested-object schema.
You might want to return an error from Reconcile if len(in) > 1
, we can always also add a CEL rule later.
@@ -59,13 +59,34 @@ func init() { | |||
ResourceKind: "SQLInstance", | |||
} | |||
|
|||
resourceContextMap["sqlinstanceauthorizednetworks"] = ResourceContext{ |
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.
Do you understand what this does? (I don't!)
I wonder if we should have this functionality in the unified_test.
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.
This was for testDriftCorrection
in the "dynamic" tests (dynamic_controller_integration_test.go
). The workflow it tests is:
deletes the resource directly on GCP and then reconciles and verifies the resource was recreated correctly
Not sure if this is covered by unified_test or not. What is the difference between the unified and dynamic tests?
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: justinsb The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
62b29c0
into
GoogleCloudPlatform:master
Change description
These are probably among the most used fields of SQLInstance resources, we should add some test cases for them.
Also, refactor SQLInstance direct controller mapper / merge code (just for
settings.IpConfiguration
, so far) to fix some issues that were discovered along the way, and to make it so that the code more closely resembles what the generated type mapping code would be (to make it easier to migrate later).ipConfiguration.ipv4Enabled
should NOT be sent to API unless explicitly specified by user.ipConfiguration.authorizedNetworks
when list sizes do not change, but values do change.