-
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
Adds support for public access prevention (beta) #5519
Adds support for public access prevention (beta) #5519
Conversation
Oops! It looks like you're using an unknown release-note type in your changelog entries:
Please only use the types listed in https://github.com/GoogleCloudPlatform/magic-modules/blob/master/.ci/RELEASE_NOTES_GUIDE.md. |
Hello! I am a robot who works on Magic Modules PRs. I have detected that you are a community contributor, so your PR will be assigned to someone with a commit-bit on this repo for initial review. Thanks for your contribution! A human will be with you soon. @melinath, please review this PR or find an appropriate assignee. |
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.
Adding some requested changes... note that beta is normally always a superset of GA unless you have some information contradicting this for this instance.
mmv1/third_party/terraform/resources/resource_storage_bucket.go.erb
Outdated
Show resolved
Hide resolved
mmv1/third_party/terraform/resources/resource_storage_bucket.go.erb
Outdated
Show resolved
Hide resolved
mmv1/third_party/terraform/resources/resource_storage_bucket.go.erb
Outdated
Show resolved
Hide resolved
mmv1/third_party/terraform/tests/resource_storage_bucket_test.go.erb
Outdated
Show resolved
Hide resolved
mmv1/third_party/terraform/tests/resource_storage_bucket_test.go.erb
Outdated
Show resolved
Hide resolved
* handled by choice of provider for resource
Oops! It looks like you're using an unknown release-note type in your changelog entries:
Please only use the types listed in https://github.com/GoogleCloudPlatform/magic-modules/blob/master/.ci/RELEASE_NOTES_GUIDE.md. |
Hey @ScottSuarez, thanks for your help so far... I've made the changes I understand. I'd like to know a bit more about the ones that aren't marked as resolved above, see questions in review. Once I'm clearer on those I can make those changes too... re-requesting review to get those answered. |
/gcbrun |
btw, I'm having trouble building the modules locally. I've followed the instructions here: https://github.com/GoogleCloudPlatform/magic-modules#preparing-magic-modules--one-time-setup, and the Building from the root, though, I get: Bundle complete! 7 Gemfile dependencies, 32 gems now installed.
Use `bundle info [gemname]` to see where a bundled gem is installed.
bundler: command not found: compiler
Install missing gem executables with `bundle install`
make[1]: *** [GNUmakefile:59: mmv1] Error 127
make[1]: Leaving directory '/mnt/c/Projects/github.com/rakuten-gcloud/magic-modules'
make: *** [GNUmakefile:55: terraform] Error 2 I wonder if this is WSL weirdness. It looks like I can get around this by running the following command directly from the bundle exec ruby compiler.rb -e terraform -o "/mnt/c/Projects/github.com/hashicorp/terraform-provider-google-beta" -v beta -p compute |
Linters are giving a load of errors in update: the same is true for the tests, only |
Ah, progress! That GA diff looks like it has a rogue deletion, hopefully quick fix coming up. |
I think we're good, let me know if anything else needs to change. I can't see any details for the one failing check, so if that's something that needs resolving and details can be pasted here that'd be good. I'll pick this up again tomorrow, heading out for today. |
/gcbrun |
I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccDatasourceGoogleServiceNetworkingPeeredDnsDomain_basic|TestAccApigeeEnvironmentIamPolicyGenerated|TestAccCloudFunctionsFunction_vpcConnector|TestAccComputeRegionNetworkEndpointGroup_regionNetworkEndpointGroupAppengineExample|TestAccContainerCluster_errorAutopilotLocation|TestAccContainerNodePool_withInvalidUpgradeSettings|TestAccStorageBucket_publicAccessPrevention You can view the result here: https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=228861 |
Looks like the test is failing in CI..
We need to read this value from the api. Please update the |
<% unless version == "ga" -%> | ||
"public_access_prevention": { | ||
Type: schema.TypeString, | ||
Optional: 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.
Most likely will need
Computed: true
here
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.
Can you explain why, and what that does? Is that to indicate it should be considered as false
if it has never been set? Happy to add that property, would just prefer to understand what I'm doing rather than cargo cult it in there.
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.
Computed means that the API can/will return this field even if it is not set.
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 is so if the field is returned in the instance that terraform does not explicitly set it. Terraform will not blow up.
We can see how the tests go to determine if they fail or not first so we can see what will be needed.
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.
Ah, so it's based on the behavior of the API itself? I guess I need to see what it returns for buckets where this is not configured. Presumably, if we see nothing as a result then I'll want to omit the computed option, but if I see something set to false, I'll want to include it. Let me have a nose around.
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.
Your understanding is correct
Looks like the |
/gcbrun |
/gcbrun |
I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccDataSourceGoogleCloudFunctionsFunction_basic|TestAccDataSourceComputeBackendBucket_basic|TestAccDataSourceGoogleStorageBucket_basic|TestAccDataSourceStorageBucketObjectContent_Basic|TestAccApigeeEnvironmentIamBindingGenerated|TestAccApigeeEnvironmentIamMemberGenerated|TestAccApigeeEnvironmentIamPolicyGenerated|TestAccCloudFunctionsCloudFunctionIamPolicyGenerated|TestAccCloudFunctionsCloudFunctionIamBindingGenerated|TestAccCloudFunctionsCloudFunctionIamMemberGenerated|TestAccIapAppEngineVersionIamBindingGenerated|TestAccIapAppEngineVersionIamMemberGenerated|TestAccIapAppEngineVersionIamPolicyGenerated|TestAccIapAppEngineServiceIamBindingGenerated|TestAccIapAppEngineVersionIamPolicyGenerated_withCondition|TestAccIapAppEngineVersionIamBindingGenerated_withCondition|TestAccIapAppEngineServiceIamMemberGenerated|TestAccIapAppEngineVersionIamMemberGenerated_withCondition|TestAccIapAppEngineServiceIamPolicyGenerated|TestAccIapAppEngineServiceIamPolicyGenerated_withCondition|TestAccIapAppEngineServiceIamBindingGenerated_withCondition|TestAccIapAppEngineServiceIamMemberGenerated_withCondition|TestAccStorageBucketIamBindingGenerated|TestAccStorageBucketIamMemberGenerated|TestAccStorageBucketIamPolicyGenerated|TestAccStorageBucketIamPolicyGenerated_withCondition|TestAccStorageBucketIamBindingGenerated_withCondition|TestAccStorageBucketIamMemberGenerated_withCondition|TestAccAppEngineServiceNetworkSettings_appEngineServiceNetworkSettingsExample|TestAccAppEngineServiceNetworkSettings_update|TestAccAppEngineServiceSplitTraffic_appEngineServiceSplitTrafficExample|TestAccAppEngineStandardAppVersion_appEngineStandardAppVersionExample|TestAccAppEngineFlexibleAppVersion_appEngineFlexibleAppVersionExample|TestAccAppEngineFlexibleAppVersion_update|TestAccAppEngineStandardAppVersion_update|TestAccAppEngineApplicationUrlDispatchRules_appEngineApplicationUrlDispatchRulesBasicExample|TestAccBigQueryJob_bigqueryJobExtractExample|TestAccBigQueryJob_bigqueryJobExtractTableReferenceExample|TestAccBigQueryTable_HivePartitioningCustomSchema|TestAccBigQueryTable_HivePartitioning|TestAccBigQueryExternalDataTable_CSV|TestAccCloudFunctionsFunction_basic|TestAccCloudFunctionsFunction_update|TestAccCloudFunctionsFunction_pubsub|TestAccCloudFunctionsFunction_bucket|TestAccCloudFunctionsFunction_vpcConnector|TestAccCloudFunctionsFunction_firestore|TestAccCloudFunctionsFunction_serviceAccountEmail|TestAccComputeBackendBucket_backendBucketBasicExample|TestAccComputeBackendBucket_backendBucketFullExample|TestAccComputeBackendBucketSignedUrlKey_basic|TestAccComputeBackendBucket_basicModified|TestAccComputeBackendBucket_withCdnPolicy|TestAccComputeRegionNetworkEndpointGroup_regionNetworkEndpointGroupFunctionsExample|TestAccComputeRegionNetworkEndpointGroup_regionNetworkEndpointGroupAppengineExample|TestAccComputeUrlMap_urlMapBasicExample|TestAccContainerCluster_errorAutopilotLocation|TestAccContainerNodePool_withInvalidUpgradeSettings|TestAccDataLossPreventionStoredInfoType_dlpStoredInfoTypeLargeCustomDictionaryExample|TestAccDataprocCluster_withStagingBucket|TestAccDataprocCluster_withTempBucket|TestAccDataprocCluster_withInitAction|TestAccDataprocCluster_withKerberos|TestAccFirebaseWebApp_firebaseWebAppBasicExample|TestAccServiceNetworkingPeeredDNSDomain_basic|TestAccLoggingBillingAccountSink_basic|TestAccLoggingBillingAccountSink_update|TestAccLoggingBillingAccountSink_described|TestAccLoggingBillingAccountSink_disabled|TestAccLoggingBillingAccountSink_heredoc|TestAccLoggingFolderSink_basic|TestAccLoggingFolderSink_described|TestAccLoggingFolderSink_disabled|TestAccLoggingFolderSink_folderAcceptsFullFolderPath|TestAccLoggingFolderSink_removeOptionals|TestAccLoggingFolderSink_update|TestAccLoggingFolderSink_heredoc|TestAccLoggingOrganizationSink_basic|TestAccLoggingOrganizationSink_update|TestAccLoggingOrganizationSink_described|TestAccLoggingOrganizationSink_disabled|TestAccLoggingOrganizationSink_heredoc|TestAccLoggingProjectSink_basic|TestAccLoggingProjectSink_described|TestAccLoggingProjectSink_described_update|TestAccLoggingProjectSink_disabled|TestAccLoggingProjectSink_updatePreservesUniqueWriter|TestAccLoggingProjectSink_heredoc|TestAccLoggingProjectSink_disabled_update|TestAccNetworkServicesEdgeCacheService_networkServicesEdgeCacheServiceBasicExample|TestAccNetworkServicesEdgeCacheService_updateAndImport|TestAccNetworkServicesEdgeCacheService_networkServicesEdgeCacheServiceAdvancedExample|TestAccStorageBucketAccessControl_storageBucketAccessControlPublicBucketExample|TestAccStorageBucketAccessControl_update|TestAccStorageBucketAcl_basic|TestAccStorageBucketAcl_upgrade|TestAccStorageBucketAcl_upgradeSingleUser|TestAccStorageBucketAcl_downgrade|TestAccStorageBucketAcl_predefined|TestAccStorageBucketAcl_unordered|TestAccStorageBucketAcl_RemoveOwner|TestAccStorageObject_basic|TestAccStorageBucketIamPolicy|TestAccStorageObject_recreate|TestAccStorageObject_content|TestAccStorageObject_withContentCharacteristics|TestAccStorageObject_dynamicContent|TestAccStorageObject_cacheControl|TestAccStorageObject_storageClass|TestAccStorageObject_metadata|TestAccStorageObject_holds|TestAccStorageObject_customerEncryption|TestAccStorageBucket_basic|TestAccStorageBucket_requesterPays|TestAccStorageBucket_lowercaseLocation|TestAccStorageObjectKms|TestAccStorageBucket_customAttributes|TestAccStorageBucket_lifecycleRuleStateArchived|TestAccStorageBucket_lifecycleRuleStateLive|TestAccStorageBucket_lifecycleRuleStateAny|TestAccStorageBucket_publicAccessPrevention|TestAccStorageBucket_update_requesterPays|TestAccStorageBucket_update|TestAccStorageBucket_forceDestroy|TestAccStorageBucket_forceDestroyWithVersioning|TestAccStorageBucket_forceDestroyObjectDeleteError|TestAccStorageBucket_logging|TestAccStorageBucket_versioning|TestAccStorageBucket_defaultEventBasedHold|TestAccStorageBucket_cors|TestAccStorageBucket_uniformBucketAccessOnly|TestAccStorageBucket_labels|TestAccStorageDefaultObjectAccessControl_storageDefaultObjectAccessControlPublicExample|TestAccStorageBucket_website|TestAccStorageDefaultObjectAccessControl_update|TestAccStorageDefaultObjectAcl_basic|TestAccStorageBucket_retentionPolicy|TestAccStorageDefaultObjectAcl_noRoleEntity|TestAccStorageBucket_retentionPolicyLocked|TestAccStorageDefaultObjectAcl_upgrade|TestAccStorageDefaultObjectAcl_downgrade|TestAccStorageDefaultObjectAcl_unordered|TestAccStorageObjectAccessControl_storageObjectAccessControlPublicObjectExample|TestAccStorageNotification_basic|TestAccStorageObjectAccessControl_update|TestAccStorageNotification_withEventsAndAttributes|TestAccStorageObjectAcl_basic|TestAccStorageObjectAccessControl_updateWithSlashes|TestAccStorageObjectAcl_upgrade|TestAccStorageObjectAcl_downgrade|TestAccStorageObjectAcl_predefined|TestAccStorageObjectAcl_predefinedToExplicit|TestAccStorageObjectAcl_explicitToPredefined|TestAccStorageObjectAcl_unordered|TestAccStorageTransferJob_omitScheduleEndDate|TestAccStorageTransferJob_basic|TestAccComputeResourceUsageExportBucket You can view the result here: https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=230046 |
Well, I obviously made things worse :) Any logs you can relay would be great. |
Yes looks like we will need the |
/gcbrun |
I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccDataSourceGoogleCloudFunctionsFunction_basic|TestAccDataSourceComputeBackendBucket_basic|TestAccDataSourceGoogleStorageBucket_basic|TestAccDataSourceStorageBucketObjectContent_Basic|TestAccApigeeEnvironmentIamMemberGenerated|TestAccApigeeEnvironmentIamPolicyGenerated|TestAccCloudFunctionsCloudFunctionIamBindingGenerated|TestAccCloudFunctionsCloudFunctionIamPolicyGenerated|TestAccCloudFunctionsCloudFunctionIamMemberGenerated|TestAccIapAppEngineVersionIamBindingGenerated|TestAccIapAppEngineVersionIamPolicyGenerated|TestAccIapAppEngineVersionIamMemberGenerated|TestAccIapAppEngineServiceIamBindingGenerated|TestAccIapAppEngineVersionIamPolicyGenerated_withCondition|TestAccIapAppEngineServiceIamMemberGenerated|TestAccIapAppEngineVersionIamBindingGenerated_withCondition|TestAccIapAppEngineVersionIamMemberGenerated_withCondition|TestAccIapAppEngineServiceIamPolicyGenerated|TestAccIapAppEngineServiceIamPolicyGenerated_withCondition|TestAccIapAppEngineServiceIamBindingGenerated_withCondition|TestAccIapAppEngineServiceIamMemberGenerated_withCondition|TestAccStorageBucketIamBindingGenerated|TestAccStorageBucketIamMemberGenerated|TestAccStorageBucketIamPolicyGenerated|TestAccStorageBucketIamPolicyGenerated_withCondition|TestAccStorageBucketIamBindingGenerated_withCondition|TestAccStorageBucketIamMemberGenerated_withCondition|TestAccAppEngineServiceNetworkSettings_appEngineServiceNetworkSettingsExample|TestAccAppEngineServiceNetworkSettings_update|TestAccAppEngineServiceSplitTraffic_appEngineServiceSplitTrafficExample|TestAccAppEngineStandardAppVersion_appEngineStandardAppVersionExample|TestAccAppEngineFlexibleAppVersion_appEngineFlexibleAppVersionExample|TestAccAppEngineFlexibleAppVersion_update|TestAccAppEngineStandardAppVersion_update|TestAccAppEngineApplicationUrlDispatchRules_appEngineApplicationUrlDispatchRulesBasicExample|TestAccBigQueryJob_bigqueryJobExtractExample|TestAccBigQueryJob_bigqueryJobExtractTableReferenceExample|TestAccBigQueryTable_HivePartitioningCustomSchema|TestAccBigQueryTable_HivePartitioning|TestAccBigQueryExternalDataTable_CSV|TestAccCloudFunctionsFunction_basic|TestAccCloudFunctionsFunction_update|TestAccCloudFunctionsFunction_pubsub|TestAccCloudFunctionsFunction_bucket|TestAccCloudFunctionsFunction_vpcConnector|TestAccCloudFunctionsFunction_firestore|TestAccCloudFunctionsFunction_serviceAccountEmail|TestAccComputeBackendBucket_backendBucketBasicExample|TestAccComputeBackendBucket_backendBucketFullExample|TestAccComputeBackendBucketSignedUrlKey_basic|TestAccComputeBackendBucket_basicModified|TestAccComputeBackendBucket_withCdnPolicy|TestAccComputeRegionNetworkEndpointGroup_regionNetworkEndpointGroupFunctionsExample|TestAccComputeRegionNetworkEndpointGroup_regionNetworkEndpointGroupAppengineExample|TestAccComputeUrlMap_urlMapBasicExample|TestAccContainerCluster_errorAutopilotLocation|TestAccContainerNodePool_withInvalidUpgradeSettings|TestAccDataLossPreventionStoredInfoType_dlpStoredInfoTypeLargeCustomDictionaryExample|TestAccDataprocCluster_withStagingBucket|TestAccDataprocCluster_withTempBucket|TestAccDataprocCluster_withInitAction|TestAccDataprocCluster_withKerberos|TestAccFirebaseWebApp_firebaseWebAppBasicExample|TestAccLoggingBillingAccountSink_basic|TestAccLoggingBillingAccountSink_update|TestAccLoggingBillingAccountSink_described|TestAccLoggingBillingAccountSink_disabled|TestAccLoggingBillingAccountSink_heredoc|TestAccLoggingFolderSink_basic|TestAccLoggingFolderSink_described|TestAccLoggingFolderSink_disabled|TestAccLoggingFolderSink_removeOptionals|TestAccLoggingFolderSink_update|TestAccLoggingFolderSink_folderAcceptsFullFolderPath|TestAccLoggingFolderSink_heredoc|TestAccLoggingOrganizationSink_basic|TestAccLoggingOrganizationSink_update|TestAccLoggingOrganizationSink_described|TestAccLoggingOrganizationSink_disabled|TestAccLoggingOrganizationSink_heredoc|TestAccLoggingProjectSink_basic|TestAccLoggingProjectSink_described|TestAccLoggingProjectSink_described_update|TestAccLoggingProjectSink_disabled|TestAccLoggingProjectSink_updatePreservesUniqueWriter|TestAccLoggingProjectSink_heredoc|TestAccLoggingProjectSink_disabled_update|TestAccNetworkServicesEdgeCacheService_networkServicesEdgeCacheServiceBasicExample|TestAccNetworkServicesEdgeCacheService_networkServicesEdgeCacheServiceAdvancedExample|TestAccNetworkServicesEdgeCacheService_updateAndImport|TestAccStorageBucketAccessControl_storageBucketAccessControlPublicBucketExample|TestAccStorageBucketAccessControl_update|TestAccStorageBucketAcl_basic|TestAccStorageBucketAcl_upgrade|TestAccStorageBucketAcl_upgradeSingleUser|TestAccStorageBucketAcl_downgrade|TestAccStorageBucketAcl_predefined|TestAccStorageBucketAcl_unordered|TestAccStorageBucketAcl_RemoveOwner|TestAccStorageBucketIamPolicy|TestAccStorageObject_basic|TestAccStorageObject_recreate|TestAccStorageObject_content|TestAccStorageObject_withContentCharacteristics|TestAccStorageObject_dynamicContent|TestAccStorageObject_cacheControl|TestAccStorageObject_storageClass|TestAccStorageObject_metadata|TestAccStorageObject_customerEncryption|TestAccStorageBucket_basic|TestAccStorageObject_holds|TestAccStorageBucket_requesterPays|TestAccStorageObjectKms|TestAccStorageBucket_lowercaseLocation|TestAccStorageBucket_customAttributes|TestAccStorageBucket_lifecycleRuleStateLive|TestAccStorageBucket_lifecycleRuleStateArchived|TestAccStorageBucket_lifecycleRuleStateAny|TestAccStorageBucket_update_requesterPays|TestAccStorageBucket_update|TestAccStorageBucket_forceDestroy|TestAccStorageBucket_forceDestroyWithVersioning|TestAccStorageBucket_versioning|TestAccStorageBucket_forceDestroyObjectDeleteError|TestAccStorageBucket_logging|TestAccStorageBucket_cors|TestAccStorageBucket_defaultEventBasedHold|TestAccStorageBucket_uniformBucketAccessOnly|TestAccStorageBucket_labels|TestAccStorageBucket_website|TestAccStorageDefaultObjectAccessControl_storageDefaultObjectAccessControlPublicExample|TestAccStorageDefaultObjectAccessControl_update|TestAccStorageDefaultObjectAcl_basic|TestAccStorageDefaultObjectAcl_noRoleEntity|TestAccStorageBucket_retentionPolicyLocked|TestAccStorageBucket_retentionPolicy|TestAccStorageDefaultObjectAcl_upgrade|TestAccStorageDefaultObjectAcl_downgrade|TestAccStorageDefaultObjectAcl_unordered|TestAccStorageObjectAccessControl_storageObjectAccessControlPublicObjectExample|TestAccStorageObjectAccessControl_update|TestAccStorageObjectAccessControl_updateWithSlashes|TestAccStorageNotification_basic|TestAccStorageNotification_withEventsAndAttributes|TestAccStorageObjectAcl_basic|TestAccStorageObjectAcl_upgrade|TestAccStorageObjectAcl_downgrade|TestAccStorageObjectAcl_predefined|TestAccStorageObjectAcl_explicitToPredefined|TestAccStorageObjectAcl_predefinedToExplicit|TestAccStorageObjectAcl_unordered|TestAccStorageTransferJob_basic|TestAccStorageTransferJob_omitScheduleEndDate|TestAccComputeResourceUsageExportBucket You can view the result here: https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=230445 |
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 like the relevant tests passed
Thanks for all your help with this! Are we not worried about the stack of other failures, then? FYI I'm now away on holiday for a week or so, but may check in on the few Google/TF PRs I have open during that time. |
fixes hashicorp/terraform-provider-google#9519
Adds Public Access Prevention support for Cloud Storage to the beta provider. See issue here:
hashicorp/terraform-provider-google#9519
and downstream PR here:
hashicorp/terraform-provider-google#10670
Tagging @ScottSuarez for review, will work through the checklist below now.
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)