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 folder-level audit configs #5282

Closed
wants to merge 1 commit into from

Conversation

ondrejklucka
Copy link

@ondrejklucka ondrejklucka commented Dec 31, 2019

Issue: #3755
Fixes #4130

@ghost ghost added the size/xl label Dec 31, 2019
@ghost ghost requested a review from paddycarver December 31, 2019 12:47
Copy link
Contributor

@chrisst chrisst left a comment

Choose a reason for hiding this comment

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

I have just done a quick pass on this but I have an initial question before I keep reviewing. Why did you choose to rewrite the google_folder_iam_policy resource from the existing resource generator to the hand written resourceGoogleFolderIamPolicy?

If that can be avoided this PR is going to be much smaller and much easier to merge in. FWIW I took your extended tests and ran them against the existing version of the resource in master and they passed as is.

@@ -115,3 +115,25 @@ func getFolderIamPolicyByFolderName(folderName string, config *Config) (*cloudre

return v1Policy, nil
}

func getFolderNameByParentAndDisplayName(parent, displayName string, config *Config) (string, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a method that is only being called by a test. It should be moved into a _test file until it's called by production code. Since it's only used by 1 test I think moving it to google/resource_google_folder_iam_audit_config_test.go is fine for now.

Type: schema.TypeBool,
Optional: true,
},
"restore_policy": {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand why you are adding new fields that are already removed.

State: resourceGoogleFolderIamPolicyImport,
},

Schema: map[string]*schema.Schema{
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be implemented by merging IamFolderSchema with IamPolicyBaseSchema as was implemented by resource_iam_policy unless there is a reason to manually specify it here.

@chrisst chrisst self-assigned this Jan 14, 2020
@paddycarver paddycarver removed their request for review March 9, 2020 18:14
@dmedora
Copy link

dmedora commented Jul 17, 2020

google_folder_iam_audit_config was merged into master in #6708, I believe this can be closed.

modular-magician added a commit to modular-magician/terraform-provider-google that referenced this pull request Oct 5, 2021
modular-magician added a commit that referenced this pull request Oct 5, 2021
@rileykarson
Copy link
Collaborator

Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 14, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for Folder Audit Config
4 participants