Gke gcsfuse #7884
Gke gcsfuse #7884
Conversation
|
Hello! I am a robot who works on Magic Modules PRs. I've detected that you're a community contributor. @megan07, 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. |
Tests analyticsTotal tests: Errors occurred during REPLAYING mode. Please fix them to complete your PR |
Run |
|
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. Terraform GA: Diff ( 5 files changed, 64 insertions(+), 5 deletions(-)) |
even after updating the go.mod and go.sum files I am still getting this error. Step #12 - "tgc-head": /go/pkg/mod/github.com/modular-magician/[email protected]/google/resource_container_cluster.go:3496:6: ac.GcsFuseCsiDriverConfig undefined (type *container.AddonsConfig has no field or method GcsFuseCsiDriverConfig) the new attribute is present in the go beta package https://pkg.go.dev/google.golang.org/api/container/v1beta1#GcsFuseCsiDriverConfig Do I need to do anything else to use this attribute ? |
Tests analyticsTotal tests: Action takenFound 14 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccComputeNetworkPeering_subnetRoutes|TestAccComputeNetworkPeering_basic|TestAccComputeNetworkPeeringRoutesConfig_networkPeeringRoutesConfigBasicExample|TestAccComputeRoute_routeIlbVipExample|TestAccComputeNetworkPeering_customRoutesUpdate|TestAccContainerCluster_withAddons|TestAccComputeFirewallPolicyRule_multipleRules|TestAccApigeeKeystoresAliasesPkcs12_ApigeeKeystoresAliasesPkcs12Example|TestAccAlloydbCluster_missingLocation|TestAccAlloydbBackup_missingLocation|TestAccDataSourceAlloydbLocations_basic|TestAccDataSourceComputeNetworkPeering_basic|TestAccDataSourceGoogleFirebaseAndroidAppConfig|TestAccApigeeKeystoresAliasesKeyCertFile_apigeeKeystoresAliasesKeyCertFileTestExample |
|
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
|
/gcbrun |
|
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. Terraform GA: Diff ( 5 files changed, 64 insertions(+), 5 deletions(-)) |
Tests analyticsTotal tests: Action takenFound 7 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccComputeFirewallPolicyRule_multipleRules|TestAccApigeeKeystoresAliasesPkcs12_ApigeeKeystoresAliasesPkcs12Example|TestAccAlloydbBackup_missingLocation|TestAccAlloydbCluster_missingLocation|TestAccApigeeKeystoresAliasesKeyCertFile_apigeeKeystoresAliasesKeyCertFileTestExample|TestAccDataSourceAlloydbLocations_basic|TestAccDataSourceGoogleFirebaseAndroidAppConfig |
|
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
|
Hi @megan07 |
| @@ -75,6 +75,7 @@ var ( | |||
| "addons_config.0.network_policy_config", | |||
| "addons_config.0.cloudrun_config", | |||
| "addons_config.0.gcp_filestore_csi_driver_config", | |||
| "addons_config.0.gcs_fuse_csi_driver_config", | |||
There was a problem hiding this comment.
This appears to only be available in beta, can we move all of this into <% unless version == 'ga' -%> blocks? You can follow how istio_config and kalm_config are handled.
There was a problem hiding this comment.
should we move only gcs_fuse_csi_driver_config to 'ga' block or should we move everything in addonsConfigKeys ?
I understand that istio_config and kalm_config are deprecated. What is the effect of moving gcs_fuse_csi_driver_config to 'ga' block ?
There was a problem hiding this comment.
Just gcs_fuse_csi_driver_config should move into the block. Typically in cases like these, I just move them into the existing blocks that contain istio_config and kalm_config, thus they are all grouped together and its easier to read IMO. It shouldn't matter that the others are deprecated, they're still in the code and will be handled in the same manner wrt the provider. Does that make sense?
Essentially the effect is that the new config block won't make it to the google provider, but only the google-beta provider. You can look at this comment above and it will show what the code will look like once they're generated downstream.
For example, you'll see that istio_config is in the google-beta provider, but it doesn't exist in the google provider. (Except for docs, we standardize on beta docs)
There was a problem hiding this comment.
Thanks for the information.
I've updated it in the latest iteration.
There was a problem hiding this comment.
thanks! and sorry for the mis-understanding, but you'll want to move all the code below that references it into blocks as well. if you do a search in the code for istio_config you'll see what i mean, i think.
…only in google-beta version.
| @@ -331,6 +332,23 @@ func ResourceContainerCluster() *schema.Resource { | |||
| }, | |||
| }, | |||
| }, | |||
| "gcs_fuse_csi_driver_config": { | |||
There was a problem hiding this comment.
For example, you'll want to move this block of code to line 423, and current line 5006 to 5056. Does that make sense? And even references in the tests.
There was a problem hiding this comment.
Sorry for that.
I have updated them all in the latest iteration. Can you please check if it looks good.
mmv1/third_party/terraform/website/docs/r/container_cluster.html.markdown
Outdated
Show resolved
Hide resolved
…ml.markdown Co-authored-by: megan07 <[email protected]>
|
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. Terraform GA: Diff ( 4 files changed, 25 insertions(+), 6 deletions(-)) |
| @@ -3920,6 +3920,9 @@ resource "google_container_cluster" "primary" { | |||
| kalm_config { | |||
| enabled = false | |||
| } | |||
| gcs_fuse_csi_driver_config { | |||
There was a problem hiding this comment.
nit: can we fix the spacing on this (and the line above it) please, to be lined up with kalm_config
| @@ -3981,6 +3984,9 @@ resource "google_container_cluster" "primary" { | |||
| kalm_config { | |||
| enabled = true | |||
| } | |||
| gcs_fuse_csi_driver_config { | |||
There was a problem hiding this comment.
nit: could we fix the spacing here too please? for both this and kalm_config
|
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. Terraform GA: Diff ( 3 files changed, 24 insertions(+), 5 deletions(-)) |
Tests analyticsTotal tests: Action takenFound 10 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccSqlDatabaseInstance_PointInTimeRecoveryEnabledForSqlServer|TestAccSqlDatabaseInstance_basicMSSQL|TestAccComputeFirewallPolicyRule_multipleRules|TestAccAlloydbBackup_missingLocation|TestAccAlloydbCluster_missingLocation|TestAccApigeeKeystoresAliasesKeyCertFile_apigeeKeystoresAliasesKeyCertFileTestExample|TestAccApigeeKeystoresAliasesPkcs12_ApigeeKeystoresAliasesPkcs12Example|TestAccSqlDatabaseInstance_rootPasswordShouldBeUpdatable|TestAccDataSourceGoogleFirebaseAndroidAppConfig|TestAccDataSourceAlloydbLocations_basic |
|
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
|
Looks like this test is failing with quota error |
|
That should be fine, it's unrelated to your change. Thanks! |
What is the process to start the merge ? can you please help me to merge this PR to magic module repo ? |
|
Done :) Thanks! |
Thank you so much @megan07. How long does it take for this to available in the terraform |
* Adding GCFuse addons attributes in GKE terraform resource. * Fixing syntax error. * Updating go.mud and go.sum files. * Moving "gcp_filestore_csi_driver_config" to "ga" block to include it only in google-beta version. * Update mmv1/third_party/terraform/website/docs/r/container_cluster.html.markdown Co-authored-by: megan07 <[email protected]> * Updating "gcs_fuse_csi_driver_config" location. * Fixing the error and moving the right gcs_fuse_csi_driver_config attribute. * Fixing spacing. --------- Co-authored-by: megan07 <[email protected]>
This PR is to add terraform support for running GCSFuse as CSI driver for GKE cluster.
If this PR is for Terraform, I acknowledge that I have:
make testandmake lintin the generated providers to ensure it passes unit and linter tests.Release Note Template for Downstream PRs (will be copied)