-
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
Add support for nodeConfig in ApigeeEnvironment #6349
Conversation
Hello! I am a robot who works on Magic Modules PRs. I've detected that you're a community contributor. @rileykarson, a repository maintainer, has been assigned to assist you and help review your changes. ❓ First time contributing? Click here for more detailsYour assigned reviewer will help review your code by:
You can help make sure that review is quick by running local tests and ensuring they're passing in between each push you make to your PR's branch. Also, try to leave a comment with each push you make, as pushes generally don't generate emails. If your reviewer doesn't get back to you within a week after your most recent change, please feel free to leave a comment on the issue asking them to take a look! In the absence of a dedicated review dashboard most maintainers manage their pending reviews through email, and those will sometimes get lost in their inbox. |
Hi Riley, this API isn't public yet (will be there later this week) so the tests won't pass for now.
Thanks! |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccComputeInstance_soleTenantNodeAffinities|TestAccActiveDirectoryPeering_activeDirectoryPeeringBasicExample|TestAccCGCSnippet_eventarcWorkflowsExample|TestAccCloudRunService_cloudRunServiceStaticOutboundExample|TestAccActiveDirectoryDomain_activeDirectoryDomainBasicExample|TestAccActiveDirectoryDomain_update|TestAccPrivatecaCertificateAuthority_privatecaCertificateAuthoritySubordinateExample |
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
Sorry, lost email on my side. I'm OOO for a bit, so picked you a new reviewer from the lottery. |
It seems that this is the first updatable field introduced to the resource. We can specify |
mmv1/products/apigee/api.yaml
Outdated
The minimum total number of gateway nodes that the is reserved for all instances that | ||
has the specified environment. If not specified, the default is determined by the | ||
recommended minimum number of nodes for that gateway. | ||
input: true |
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.
If this is updatable, you do not want input: true
, as this makes the field ForceNew
The additional complication is that, actually there is already a PUT call that can be used to update all the other fields (except for this nodeConfig) of this resource (ref: https://cloud.google.com/apigee/docs/reference/apis/apigee/rest/v1/organizations.environments/update), and this new "nodeConfig" can only be updated via PATCH. |
I see. I believe you can set the |
Thanks, I added When I tested it locally and updated the nodeConfig, it failed with a PUT call failure. But when I commented out the PUT call (above line 285 and a few related lines), the PATCH was fired properly and the update succeeded. Is there a way to not fire the PUT call when only the nodeConfig has changed? |
Apologies, that was not expected. I'll have to take a look myself to see how to best handle this situation as it's fairly rare. Would you mind pushing the changes you had in MM locally that ended up failing? So I can pull the change and test things out on my end? |
Sure, pushed, thanks! |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccComputeInstance_soleTenantNodeAffinities|TestAccComputeInstance_networkPerformanceConfig|TestAccComputeGlobalForwardingRule_internalLoadBalancing|TestAccCloudfunctions2function_cloudfunctions2BasicAuditlogsExample|TestAccCloudfunctions2function_cloudfunctions2BasicGcsExample|TestAccCloudFunctions2Function_fullUpdate|TestAccCGCSnippet_eventarcWorkflowsExample|TestAccCloudRunService_cloudRunServiceStaticOutboundExample|TestAccPrivatecaCertificateAuthority_privatecaCertificateAuthoritySubordinateExample |
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
@xuchenma This is an API constraint that I don't believe we've run into before for MMv1 generated resources. My initial impression is that this should be fixed in the API itself. I imagine TF is likely one of the top consumers of the API, and this constraint is not helpful. Surely this must be an implementation issue and not an intended design decision? I don't know if you have any insight into that. Attempting to work around this in MMv1 generation will be relatively unsatisfactory. How we have the PR now excludes
This should allow the The only non-compromising way to implement this would be to convert the resource to be completely handwritten with very specific custom logic to deal with this. |
example an reference to the file in |
I believe the logic within the update encoder file will look like
|
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 2 files changed, 188 insertions(+), 5 deletions(-)) |
Thanks Cameron! I made the changes and it worked for me locally. Please check. There are a few complications:
|
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccFirebaserulesRelease_BasicRelease|TestAccComputeInstance_networkPerformanceConfig|TestAccCloudfunctions2function_cloudfunctions2BasicGcsExample|TestAccComputeGlobalForwardingRule_internalLoadBalancing|TestAccCloudFunctions2Function_fullUpdate|TestAccComputeInstance_soleTenantNodeAffinities|TestAccCloudfunctions2function_cloudfunctions2BasicAuditlogsExample|TestAccCloudRunService_cloudRunServiceStaticOutboundExample|TestAccCGCSnippet_eventarcWorkflowsExample|TestAccPrivatecaCertificateAuthority_privatecaCertificateAuthoritySubordinateExample|TestAccSqlDatabaseInstance_authNets|TestAccSqlDatabaseInstance_withPrivateNetwork_withAllocatedIpRange|TestAccSqlDatabaseInstance_settings_upgrade|TestAccSqlDatabaseInstance_basic_with_user_labels|TestAccSqlDatabaseInstance_settings_deletionProtection|TestAccSqlDatabaseInstance_basicMSSQL|TestAccSqlDatabaseInstance_PointInTimeRecoveryEnabled|TestAccSqlDatabaseInstance_SqlServerAuditConfig |
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
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.
Looks good so far, but unfortunately I'm only just now noticing that we do not have explicit tests for update. We need to add a handwritten test that updates node_config
to ensure that it works now and will continue working in the future.
https://github.com/GoogleCloudPlatform/magic-modules/blob/main/mmv1/third_party/terraform/resources/resource_cloudfunction2_function_test.go.erb should be a very similar example, with further documentation at https://github.com/GoogleCloudPlatform/magic-modules/tree/main/mmv1/third_party/terraform#update-tests
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 3 files changed, 189 insertions(+), 5 deletions(-)) |
Thanks for the review! I added a test for nodeConfig update; in my local I can see the test is calling PATCH correctly. However as I mentioned in the above comment #6349 (comment), PUT is a sync call, but PATCH is an async call. Because I removed the async check for update (otherwise PUT is broken), in the test, after calling PATCH, it immediately calls GET to verify the state, while the state has not changed yet. |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccSqlDatabaseInstance_withPrivateNetwork_withAllocatedIpRange|TestAccPrivatecaCertificateAuthority_privatecaCertificateAuthoritySubordinateExample|TestAccSqlDatabaseInstance_SqlServerAuditConfig|TestAccComputeInstance_networkPerformanceConfig|TestAccComputeInstance_soleTenantNodeAffinities|TestAccCloudfunctions2function_cloudfunctions2BasicGcsExample|TestAccCloudFunctions2Function_fullUpdate|TestAccCloudfunctions2function_cloudfunctions2BasicAuditlogsExample|TestAccCloudRunService_cloudRunServiceStaticOutboundExample|TestAccComputeGlobalForwardingRule_internalLoadBalancing|TestAccCGCSnippet_eventarcWorkflowsExample |
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
@xuchenma I see, sorry for not catching that in your earlier message. we are really trying to stretch the MMv1 template with this logic here, and I am getting close to just recommending you to copy the generated files and convert the resource to handwritten so you can tweak the go code specifically. However, there is another attempt I can think of. This is the resource template where we write the logic for fields with custom updates: https://github.com/GoogleCloudPlatform/magic-modules/blob/main/mmv1/templates/terraform/resource.erb#L702 |
Hi @xuchenma, I have spoken with Riley and he is also open to moving this to a handwritten resource if needed. To be frank, I cannot be 100% confident that my previous comment would also not produce some sort of complication as I have been wrong up to this point. I apologize for the amount of iterations this has caused you, but this really is behavior we have not run into otherwise. It's up to you if you want to explore my earlier suggestion, but know that converting to handwritten will give you full control of the go code and let you tweak the update function to exactly what is needed here. You would not be starting from scratch as you could re-use the majority of the generated code as shown in https://github.com/modular-magician/terraform-provider-google-beta/compare/auto-pr-6349-old..auto-pr-6349 . There's some extra steps like manually registering the resource into https://github.com/GoogleCloudPlatform/magic-modules/blob/main/mmv1/third_party/terraform/utils/provider.go.erb#L377 |
Thanks Cameron and Riley. I tried to change the update_verb of ApigeeEnvironment resource to PATCH just now, it seems to be working for nodeConfig. However, setting update_verb to PUT for other fields doesn't seem to generate another PUT codeblock in resource_apigee_environment.go, so there could be more complications. But as I look deeper, I think with our current implementation for ApigeeEnvironment in mmv1, we can't really make a valid update call. All the fields except for "name" is annotated with "input: true" which results in ForceNew when its value changes, while making changes to "name" is not allowed in the API backend (we should add "input: true" to "name" too). So, effectively no update call can be made to ApigeeEnvironment. Although the API supports updating some of the fields like "properties", such field is not added to mmv1 yet. I think we should go for handwritten resource when we need to support PUT updates to ApigeeEnvironment for new fields, but right now we can just set the update_verb of the resource as PATCH since it is not a breaking change. What do you think? |
That sounds reasonable for now. There's a chance the |
Thanks, updated! |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 3 files changed, 154 insertions(+), 28 deletions(-)) |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccFirebaserulesRelease_BasicRelease|TestAccComputeInstance_soleTenantNodeAffinities|TestAccComputeGlobalForwardingRule_internalLoadBalancing|TestAccCloudfunctions2function_cloudfunctions2BasicAuditlogsExample|TestAccCloudFunctions2Function_fullUpdate|TestAccCloudfunctions2function_cloudfunctions2BasicGcsExample|TestAccCloudRunService_cloudRunServiceScheduledExample|TestAccCGCSnippet_storageObjectLifecycleSettingExample|TestAccCloudRunService_cloudRunServiceStaticOutboundExample|TestAccSqlDatabaseInstance_withPrivateNetwork_withAllocatedIpRange|TestAccSqlUser_mysqlDisabled|TestAccSqlDatabaseInstance_SqlServerAuditConfig|TestAccPrivatecaCertificateAuthority_privatecaCertificateAuthoritySubordinateExample |
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
Running Apigee tests in test build since they don't run in VCR |
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.
All tests passed including the new update. Generated code looks good to me. Thanks for your efforts on this @xuchenma !
This PR solves hashicorp/terraform-provider-google#12218
If this PR is for Terraform, I acknowledge that I have:
make test
andmake lint
to ensure it passes unit and linter tests.Release Note Template for Downstream PRs (will be copied)