Skip to content
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 GCS managed folders #10786

Merged
merged 19 commits into from
Jun 26, 2024

Conversation

juliocc
Copy link
Contributor

@juliocc juliocc commented May 24, 2024

This PR adds support for GCS Managed Folders

Related Issue : hashicorp/terraform-provider-google#18053

Release Note Template for Downstream PRs (will be copied)

`google_storage_managed_folder`
`google_storage_managed_folder_iam`

@github-actions github-actions bot requested a review from trodge May 24, 2024 09:30
Copy link

Hello! I am a robot. Tests will require approval from a repository maintainer to run.

@trodge, 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.

@juliocc
Copy link
Contributor Author

juliocc commented May 24, 2024

I just noticed that #10670 was also addressing the same issue but it seems inactive for the last few days.

@modular-magician
Copy link
Collaborator

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

google provider: Diff ( 8 files changed, 1679 insertions(+), 3 deletions(-))
google-beta provider: Diff ( 8 files changed, 1679 insertions(+), 3 deletions(-))
terraform-google-conversion: Diff ( 4 files changed, 430 insertions(+))
Open in Cloud Shell: Diff ( 8 files changed, 227 insertions(+))

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 104
Passed tests: 88
Skipped tests: 8
Affected tests: 8

Click here to see the affected service packages
  • storage

Non-exercised tests

Tests were added that are skipped in VCR:

  • TestAccStorageManagedFolderIamBindingGenerated_withAndWithoutCondition
  • TestAccStorageManagedFolderIamMemberGenerated_withAndWithoutCondition

Action taken

Found 8 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
TestAccStorageManagedFolderIamBindingGenerated|TestAccStorageManagedFolderIamBindingGenerated_withCondition|TestAccStorageManagedFolderIamMemberGenerated|TestAccStorageManagedFolderIamMemberGenerated_withCondition|TestAccStorageManagedFolderIamPolicyGenerated|TestAccStorageManagedFolderIamPolicyGenerated_withCondition|TestAccStorageManagedFolder_storageManagedFolderBasicExample|TestAccStorageManagedFolder_storageManagedFolderNestedExample

Get to know how VCR tests work

@modular-magician
Copy link
Collaborator

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccStorageManagedFolderIamBindingGenerated[Debug log]
TestAccStorageManagedFolderIamBindingGenerated_withCondition[Debug log]
TestAccStorageManagedFolderIamMemberGenerated[Debug log]
TestAccStorageManagedFolderIamMemberGenerated_withCondition[Debug log]
TestAccStorageManagedFolderIamPolicyGenerated[Debug log]
TestAccStorageManagedFolderIamPolicyGenerated_withCondition[Debug log]
TestAccStorageManagedFolder_storageManagedFolderBasicExample[Debug log]
TestAccStorageManagedFolder_storageManagedFolderNestedExample[Debug log]

$\textcolor{green}{\textsf{No issues found for passed tests after REPLAYING rerun.}}$


$\textcolor{green}{\textsf{All tests passed!}}$
View the build log or the debug log for each test

@github-actions github-actions bot requested a review from trodge May 24, 2024 21:07
@modular-magician
Copy link
Collaborator

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

google provider: Diff ( 8 files changed, 1681 insertions(+), 3 deletions(-))
google-beta provider: Diff ( 8 files changed, 1681 insertions(+), 3 deletions(-))
terraform-google-conversion: Diff ( 4 files changed, 430 insertions(+))
Open in Cloud Shell: Diff ( 8 files changed, 228 insertions(+))

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 104
Passed tests: 94
Skipped tests: 8
Affected tests: 2

Click here to see the affected service packages
  • storage

Non-exercised tests

Tests were added that are skipped in VCR:

  • TestAccStorageManagedFolderIamBindingGenerated_withAndWithoutCondition
  • TestAccStorageManagedFolderIamMemberGenerated_withAndWithoutCondition

Action taken

Found 2 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
TestAccStorageNotification_basic|TestAccStorageNotification_withEventsAndAttributes

Get to know how VCR tests work

@modular-magician
Copy link
Collaborator

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccStorageNotification_basic[Debug log]
TestAccStorageNotification_withEventsAndAttributes[Debug log]

$\textcolor{green}{\textsf{No issues found for passed tests after REPLAYING rerun.}}$


$\textcolor{green}{\textsf{All tests passed!}}$
View the build log or the debug log for each test

Copy link

This PR has been waiting for review for 2 weekdays. Please take a look! Use the label disable-review-reminders to disable these notifications.

Copy link

@GoogleCloudPlatform/terraform-team This PR has been waiting for review for 1 week. Please take a look! Use the label disable-review-reminders to disable these notifications.

2 similar comments
Copy link

github-actions bot commented Jun 1, 2024

@GoogleCloudPlatform/terraform-team This PR has been waiting for review for 1 week. Please take a look! Use the label disable-review-reminders to disable these notifications.

Copy link

github-actions bot commented Jun 2, 2024

@GoogleCloudPlatform/terraform-team This PR has been waiting for review for 1 week. Please take a look! Use the label disable-review-reminders to disable these notifications.

Copy link

github-actions bot commented Jun 7, 2024

@GoogleCloudPlatform/terraform-team This PR has been waiting for review for 2 weeks. Please take a look! Use the label disable-review-reminders to disable these notifications.

@kautikdk
Copy link
Member

Hi @trodge, Can you please prioritize this PR review?

@ludoo
Copy link

ludoo commented Jun 11, 2024

I have a customer who's been waiting for this, what's the blocker with @juliocc changes, which looked like they were ready to go?

@melinath
Copy link
Member

@trodge are there any blockers here? If this implementation is complete it would be great to get it merged; the other PR seems to have stalled out.

Copy link
Member

@melinath melinath left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requested some changes below.

mmv1/products/storage/ManagedFolder.yaml Outdated Show resolved Hide resolved
mmv1/products/storage/ManagedFolder.yaml Outdated Show resolved Hide resolved
mmv1/products/storage/ManagedFolder.yaml Show resolved Hide resolved
mmv1/products/storage/ManagedFolder.yaml Show resolved Hide resolved
mmv1/products/storage/ManagedFolder.yaml Show resolved Hide resolved
mmv1/products/storage/ManagedFolder.yaml Outdated Show resolved Hide resolved
mmv1/products/storage/ManagedFolder.yaml Outdated Show resolved Hide resolved
mmv1/products/storage/ManagedFolder.yaml Outdated Show resolved Hide resolved
@melinath
Copy link
Member

@kautikdk I don't think it needs to be a blocker for this PR but it might be worth adding a flag to this resource to allow deletion of nonempty managed folders, similar to google_storage_bucket.force_destroy. Looks like for managed folders the query param is called allowNonEmpty but that wouldn't really track as a resource field name - maybe either delete_allow_non_empty or force_destroy (to match google_storage_bucket.force_destroy)?

If the priority is getting this resource out we can ship it without that field and add it in a separate PR, since it may introduce some complications.

@kautikdk
Copy link
Member

kautikdk commented Jun 13, 2024

Hi @melinath, Got your point. It make sense to add new field to force destroy managed folder even if it is non-empty. The field would be more of like virtual field as we won't be sent field in the INSERT/PATCH methods and won't get it from the response as well. It will be only used in deletion of managed folder. I am not sure how virtual fields are being implemented in MMV1 resources but if It's not possible to add it in the MMV1 resource without modifying 2 or more CRUD methods then we might have to move this resource to the hand-written resources.
As we know it's query parameter and not a blocker of delete method, We can track it in the separate Feature Request. We will prioritize this and add support ASAP.

@melinath
Copy link
Member

@kautikdk SGTM. We do have support for virtual fields like force_delete in mmv1 though it is not a well documented process. Definitely reach out if you have further questions while working on the new ticket; don't want to take up too much space here.

@modular-magician
Copy link
Collaborator

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

google provider: Diff ( 7 files changed, 1582 insertions(+), 2 deletions(-))
google-beta provider: Diff ( 7 files changed, 1582 insertions(+), 2 deletions(-))
terraform-google-conversion: Diff ( 1 file changed, 79 insertions(+))
Open in Cloud Shell: Diff ( 4 files changed, 111 insertions(+))

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 105
Passed tests: 89
Skipped tests: 9
Affected tests: 7

Click here to see the affected service packages
  • storage
#### Non-exercised tests

Tests were added that are skipped in VCR:

  • TestAccStorageManagedFolderIamBindingGenerated_withAndWithoutCondition
  • TestAccStorageManagedFolderIamMemberGenerated_withAndWithoutCondition

Action taken

Found 7 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
  • TestAccStorageManagedFolderIamBindingGenerated
  • TestAccStorageManagedFolderIamBindingGenerated_withCondition
  • TestAccStorageManagedFolderIamMemberGenerated
  • TestAccStorageManagedFolderIamMemberGenerated_withCondition
  • TestAccStorageManagedFolderIamPolicyGenerated
  • TestAccStorageManagedFolderIamPolicyGenerated_withCondition
  • TestAccStorageManagedFolder_storageManagedFolderBasicExample

Get to know how VCR tests work

@modular-magician
Copy link
Collaborator

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccStorageManagedFolderIamBindingGenerated[Debug log]
TestAccStorageManagedFolderIamBindingGenerated_withCondition[Debug log]
TestAccStorageManagedFolderIamMemberGenerated[Debug log]
TestAccStorageManagedFolderIamMemberGenerated_withCondition[Debug log]
TestAccStorageManagedFolderIamPolicyGenerated[Debug log]
TestAccStorageManagedFolderIamPolicyGenerated_withCondition[Debug log]
TestAccStorageManagedFolder_storageManagedFolderBasicExample[Debug log]

$\textcolor{green}{\textsf{No issues found for passed tests after REPLAYING rerun.}}$


$\textcolor{green}{\textsf{All tests passed!}}$

View the build log or the debug log for each test

@github-actions github-actions bot requested a review from melinath June 25, 2024 23:41
@modular-magician
Copy link
Collaborator

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

google provider: Diff ( 7 files changed, 1582 insertions(+), 2 deletions(-))
google-beta provider: Diff ( 7 files changed, 1582 insertions(+), 2 deletions(-))
terraform-google-conversion: Diff ( 1 file changed, 79 insertions(+))
Open in Cloud Shell: Diff ( 4 files changed, 111 insertions(+))

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 105
Passed tests: 96
Skipped tests: 9
Affected tests: 0

Click here to see the affected service packages
  • storage
#### Non-exercised tests

Tests were added that are skipped in VCR:

  • TestAccStorageManagedFolderIamBindingGenerated_withAndWithoutCondition
  • TestAccStorageManagedFolderIamMemberGenerated_withAndWithoutCondition
    $\textcolor{green}{\textsf{All tests passed!}}$

View the build log

Copy link
Member

@melinath melinath left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this should be good to go.

@kautikdk
Copy link
Member

@juliocc Thank you very much for the contribution! Hope you will contribute again in future.
@melinath Thank you for your guidance throughout the process.

BBBmau pushed a commit to BBBmau/magic-modules that referenced this pull request Jun 28, 2024
Co-authored-by: Stephen Lewis (Burrows) <stephen.r.burrows@gmail.com>
pcostell pushed a commit to pcostell/magic-modules that referenced this pull request Jul 16, 2024
Co-authored-by: Stephen Lewis (Burrows) <stephen.r.burrows@gmail.com>
vijaykanthm pushed a commit to vijaykanthm/magic-modules that referenced this pull request Jul 22, 2024
Co-authored-by: Stephen Lewis (Burrows) <stephen.r.burrows@gmail.com>
Charlesleonius pushed a commit to Charlesleonius/magic-modules that referenced this pull request Aug 1, 2024
Co-authored-by: Stephen Lewis (Burrows) <stephen.r.burrows@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants