-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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 google_logging_project_exclusion resource #990
Conversation
bf4280f
to
b8cf602
Compare
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 overall! When adding some of the other resources, you might be interested in some of the work done with IAM around repeated code. The logic here is much simpler than that so I don't think it's quite as necessary here, but if you're curious take a look at iam*.go. For this first resource though, I think we should keep it as-is, just with a few small improvements :)
"github.com/hashicorp/terraform/helper/resource" | ||
) | ||
|
||
func TestAccLoggingProjectExclusion_importBasic(t *testing.T) { |
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.
We've just started trying to get away from separating out our import tests, since it usually means we end up essentially running the exact same test twice without realizing it. Mind just moving the import check into the resource test?
google/resource_logging_exclusion.go
Outdated
Optional: true, | ||
}, | ||
|
||
"filter": { |
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.
Another thing we're trying to be more consistent about these days is sorting our fields as Required -> Optional -> Computed, and alphabetically within each.
return handleNotFoundError(err, d, fmt.Sprintf("Project Logging Exclusion %s", d.Get("name").(string))) | ||
} | ||
|
||
flattenResourceLoggingExclusion(d, exclusion) |
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.
You should also set the project here in case it was imported from a project that isn't the provider default (and then consequently, set "project" to computed in the schema)
} | ||
|
||
func toBool(attribute string) (bool, error) { | ||
// Handle the case where an unset value defaults to false |
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.
nit: This can be made a bit simpler:
if attribute == "" {
return false, nil
}
return strconv.ParseBool(attribute)
The error message that ParseBool returns for an invalid param is strconv.ParseBool: parsing "not-a-bool": invalid syntax
which I think is plenty descriptive, especially given that it's just a test failure (as opposed to an end-user message)
|
||
Manages a project-level logging exclusion. For more information see | ||
[the official documentation](https://cloud.google.com/logging/docs/) and | ||
[Exclusing Logs](https://cloud.google.com/logging/docs/exclusions). |
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.
typo: Excluding
[Exclusing Logs](https://cloud.google.com/logging/docs/exclusions). | ||
|
||
Note that you must have the "Logs Configuration Writer" IAM role (`roles/logging.configWriter`) | ||
granted to the credentials used with terraform. |
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.
nit: Terraform
|
||
```hcl | ||
resource "google_logging_project_exclusion" "my-exclusion" { | ||
name = "my-pubsub-instance-exclusion" |
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.
I know it's just an example config but it does seem a bit out of place to have the name be about pubsub and the description/filter be about GCE
b8cf602
to
e61cc39
Compare
@danawillow thanks the detailed review! I have taken a look at IAM and tried to find a similar abstraction for log exclusions. I'm still not 100% happy with the variable naming and the splitting into different source files. If you have good advice here I'd be really happy to improve that further. Apart from that I have now added folder-level log exclusions using the new abstraction. It would be great if you could do a second round of review before I add the missing documentation and the missing implementations for
|
Oh, and I will of course squash all commits into one when I'm done… I just left the change commits there for potentially easier review. |
|
||
flattenResourceLoggingExclusion(d, exclusion) | ||
|
||
if updater.GetResourceType() == "projects" { |
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.
Not sure about putting this special case in here?!
return &exclusion, updateMask | ||
} | ||
|
||
// The ResourceLoggingExclusionUpdater interface is implemented for each GCP |
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.
Should the stuff that comes below go to a separate file? If so, how should it be called?
|
||
// Textual description of this resource to be used in error message. | ||
// The description should include the unique resource identifier. | ||
DescribeResource() string |
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 was more or less copied from IAM. Not sure if it makes entirely sense like that.
false. | ||
|
||
* `project` - (Optional) The project to create the exclusion in. If omitted, the project associated with the provider is | ||
used. |
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.
@danawillow would you like me to apply the Required -> Optional -> Computed and alphabetical in the groups ordering here in the docs as well?
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.
Yup, thanks for doing that.
@danawillow any chance you could have a second look before I add log-exclusions for organizations and billing? |
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.
I think I want to backtrack on my previous suggestion. Although this way is less repeating-ourselves-ey, it feels somewhat overcomplicated. I think it might be better to go back to how it originally was and just have these look like typical resources. What do you think?
google/logging_exclusion_folder.go
Outdated
func (u *FolderLoggingExclusionUpdater) CreateLoggingExclusion(parent string, exclusion *logging.LogExclusion) error { | ||
_, err := u.Config.clientLogging.Folders.Exclusions.Create(parent, exclusion).Do() | ||
if err != nil { | ||
return errwrap.Wrap(fmt.Errorf("Error creating logging exclusion for %s.", u.DescribeResource()), err) |
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.
You should be able to just use errwrap.Wrapf here
google/resource_logging_exclusion.go
Outdated
} | ||
} | ||
|
||
func ResourceLoggingExclusionWithImport(parentSpecificSchema map[string]*schema.Schema, newUpdaterFunc newResourceLoggingExclusionUpdaterFunc) *schema.Resource { |
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.
Since I think they'll all support import, let's just do one ResourceLoggingExclusion()
that has the import functionality (if you decide to keep this version)
PreCheck: func() { testAccPreCheck(t) }, | ||
Providers: testAccProviders, | ||
CheckDestroy: testAccCheckLoggingFolderExclusionDestroy, | ||
Steps: []resource.TestStep{ |
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 also add an import step here to test import?
Config: testAccLoggingProjectExclusion_basic(exclusionName), | ||
}, | ||
|
||
resource.TestStep{ |
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.
Since you already have a test that sets up infrastructure with _basic, we can save a bit of time in the test suite by adding this step into the _basic test.
false. | ||
|
||
* `project` - (Optional) The project to create the exclusion in. If omitted, the project associated with the provider is | ||
used. |
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.
Yup, thanks for doing that.
@ctavan Are you still interested in getting this merged? If you're coming back around to it (no rush!) I'm happy to leave the issue open, but if not, I'll close it as stale in a little while. |
@ndmckinley yes I’m still planning to finish this, I just had a longer break from work. I hope I can finish this over the next few weeks. |
e28f5f4
to
8412599
Compare
|
5191157
to
8623f55
Compare
@ndmckinley @danawillow I've taken another look at this.
Apart from that everything works well for me so far. |
8623f55
to
dce86ba
Compare
@pdecat I just saw your comment in: #1467 (comment) Does this apply to logging exclusions as well? |
Hi @ctavan, I can't really tell, I'm just an occasional contributor. |
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.
Thanks for doing this, @ctavan! I'm fine with accepting this PR. It'll take some time before we'll be able to magic-modules-ify this API, so this will allow people to use the resources until then (and give you some credit for working hard on them!) One comment about tests, otherwise looks good.
Check: resource.ComposeTestCheckFunc( | ||
testAccCheckLoggingBillingAccountExclusionExists("google_logging_billing_account_exclusion.basic", &exclusionAfter), | ||
testAccCheckLoggingBillingAccountExclusion(&exclusionAfter, "google_logging_billing_account_exclusion.basic"), | ||
), |
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 add an import step here as well? We're trying to move away from custom checks and towards import-as-the-verifier (see #778 for how it all works)
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.
That was actually a good one! 👍
Not here, but in the folder test:
testing.go:513: Step 1 error: ImportStateVerify attributes not equivalent. Difference is shown below. Top is actual, bottom is expected.
(map[string]string) (len=1) {
(string) (len=6) "folder": (string) (len=12) "355928736271"
}
(map[string]string) (len=1) {
(string) (len=6) "folder": (string) (len=20) "folders/355928736271"
}
Will submit the updates shortly.
@danawillow thanks for the feedback! No worries, I'm not so much after the credit, rather after being able to use this feature with the upstream terraform google provider ;). Your comment on the test import made me stumble upon an issue: The
I was copying the interface but that actually poses a problem for the import: When importing the resource I don't know which notation I should use. Hence the test fails with:
This left me with a few questions:
So I fear I need some help from you before I can finally finish up this PR. Thanks in advance! |
Good question! I've run into similar scenarios a few times, but I can't remember if I had any clever way of solving it. I think I would just do an |
dce86ba
to
1f26768
Compare
@danawillow thanks for the suggestion. I just tried to do what you described but ran into another issue: As far as I understand, the Meaning, the actual state is not modified by the import, so I don't see how I went a different route and used Apart from this I hope I have finally all things covered. I also run the acceptance tests again, output is in the commit message. |
1f26768
to
1224341
Compare
Adds support for log exclusions in billingAccounts, organizations, folders and projects, see: https://cloud.google.com/logging/docs/exclusions ``` ==> Checking that code complies with gofmt requirements... TF_ACC=1 go test ./google -v -run=Exclusion -timeout 120m === RUN TestAccLoggingBillingAccountExclusion_basic === PAUSE TestAccLoggingBillingAccountExclusion_basic === RUN TestAccLoggingBillingAccountExclusion_update === PAUSE TestAccLoggingBillingAccountExclusion_update === RUN TestAccLoggingFolderExclusion_basic === PAUSE TestAccLoggingFolderExclusion_basic === RUN TestAccLoggingFolderExclusion_folderAcceptsFullFolderPath === PAUSE TestAccLoggingFolderExclusion_folderAcceptsFullFolderPath === RUN TestAccLoggingFolderExclusion_update === PAUSE TestAccLoggingFolderExclusion_update === RUN TestAccLoggingOrganizationExclusion_basic === PAUSE TestAccLoggingOrganizationExclusion_basic === RUN TestAccLoggingOrganizationExclusion_update === PAUSE TestAccLoggingOrganizationExclusion_update === RUN TestAccLoggingProjectExclusion_basic === PAUSE TestAccLoggingProjectExclusion_basic === RUN TestAccLoggingProjectExclusion_disablePreservesFilter === PAUSE TestAccLoggingProjectExclusion_disablePreservesFilter === RUN TestAccLoggingProjectExclusion_update === PAUSE TestAccLoggingProjectExclusion_update === CONT TestAccLoggingBillingAccountExclusion_basic === CONT TestAccLoggingOrganizationExclusion_update === CONT TestAccLoggingProjectExclusion_update === CONT TestAccLoggingFolderExclusion_folderAcceptsFullFolderPath === CONT TestAccLoggingOrganizationExclusion_basic --- PASS: TestAccLoggingProjectExclusion_update (3.60s) --- PASS: TestAccLoggingOrganizationExclusion_update (4.40s) === CONT TestAccLoggingFolderExclusion_update --- PASS: TestAccLoggingOrganizationExclusion_basic (1.90s) === CONT TestAccLoggingFolderExclusion_basic --- PASS: TestAccLoggingBillingAccountExclusion_basic (6.21s) === CONT TestAccLoggingBillingAccountExclusion_update --- PASS: TestAccLoggingBillingAccountExclusion_update (5.90s) === CONT TestAccLoggingProjectExclusion_disablePreservesFilter --- PASS: TestAccLoggingProjectExclusion_disablePreservesFilter (3.90s) === CONT TestAccLoggingProjectExclusion_basic --- PASS: TestAccLoggingFolderExclusion_folderAcceptsFullFolderPath (16.67s) --- PASS: TestAccLoggingProjectExclusion_basic (1.96s) --- PASS: TestAccLoggingFolderExclusion_basic (15.30s) --- PASS: TestAccLoggingFolderExclusion_update (18.35s) PASS ok github.com/terraform-providers/terraform-provider-google/google 22.810s ```
1224341
to
2c2e77c
Compare
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.
Sure, that's fair. Thanks @ctavan! I'll go ahead and merge this now.
(also in the future, if you could avoid squashing your commits, that makes it easier for me to review so I can see what changed in between comments. No worries for this time though)
Oops, sorry for squashing the fixups rightaway. Do you usually squash upon merge? Then I'll of course keep the commits after review rounds! Anyways, thanks for your patience with the reviews and thanks for merging @danawillow ! |
Yup! No problem! |
Adds support for log exclusions in billingAccounts, organizations, folders and projects, see: https://cloud.google.com/logging/docs/exclusions ``` ==> Checking that code complies with gofmt requirements... TF_ACC=1 go test ./google -v -run=Exclusion -timeout 120m === RUN TestAccLoggingBillingAccountExclusion_basic === PAUSE TestAccLoggingBillingAccountExclusion_basic === RUN TestAccLoggingBillingAccountExclusion_update === PAUSE TestAccLoggingBillingAccountExclusion_update === RUN TestAccLoggingFolderExclusion_basic === PAUSE TestAccLoggingFolderExclusion_basic === RUN TestAccLoggingFolderExclusion_folderAcceptsFullFolderPath === PAUSE TestAccLoggingFolderExclusion_folderAcceptsFullFolderPath === RUN TestAccLoggingFolderExclusion_update === PAUSE TestAccLoggingFolderExclusion_update === RUN TestAccLoggingOrganizationExclusion_basic === PAUSE TestAccLoggingOrganizationExclusion_basic === RUN TestAccLoggingOrganizationExclusion_update === PAUSE TestAccLoggingOrganizationExclusion_update === RUN TestAccLoggingProjectExclusion_basic === PAUSE TestAccLoggingProjectExclusion_basic === RUN TestAccLoggingProjectExclusion_disablePreservesFilter === PAUSE TestAccLoggingProjectExclusion_disablePreservesFilter === RUN TestAccLoggingProjectExclusion_update === PAUSE TestAccLoggingProjectExclusion_update === CONT TestAccLoggingBillingAccountExclusion_basic === CONT TestAccLoggingOrganizationExclusion_update === CONT TestAccLoggingProjectExclusion_update === CONT TestAccLoggingFolderExclusion_folderAcceptsFullFolderPath === CONT TestAccLoggingOrganizationExclusion_basic --- PASS: TestAccLoggingProjectExclusion_update (3.60s) --- PASS: TestAccLoggingOrganizationExclusion_update (4.40s) === CONT TestAccLoggingFolderExclusion_update --- PASS: TestAccLoggingOrganizationExclusion_basic (1.90s) === CONT TestAccLoggingFolderExclusion_basic --- PASS: TestAccLoggingBillingAccountExclusion_basic (6.21s) === CONT TestAccLoggingBillingAccountExclusion_update --- PASS: TestAccLoggingBillingAccountExclusion_update (5.90s) === CONT TestAccLoggingProjectExclusion_disablePreservesFilter --- PASS: TestAccLoggingProjectExclusion_disablePreservesFilter (3.90s) === CONT TestAccLoggingProjectExclusion_basic --- PASS: TestAccLoggingFolderExclusion_folderAcceptsFullFolderPath (16.67s) --- PASS: TestAccLoggingProjectExclusion_basic (1.96s) --- PASS: TestAccLoggingFolderExclusion_basic (15.30s) --- PASS: TestAccLoggingFolderExclusion_update (18.35s) PASS ok github.com/terraform-providers/terraform-provider-google/google 22.810s ```
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 hashibot-feedback@hashicorp.com. Thanks! |
Fixes #1404
Adds support for project-level log exclusions, see:
https://cloud.google.com/logging/docs/exclusions
Are you interested in merging this? If so, I could provide additional resources for organizations, folders and billing.
I'm still a bit concerned about some code duplication (like all the stuff in
logging_utils
and thetoBool
function which contains logic from https://github.com/terraform-providers/terraform-provider-google/blob/0133845a19ee292d213ce3607c9550431daeb084/google/resource_container_cluster_test.go#L974-L992 ), so if you have suggestion on how to DRY this a bit, I'd be happy to take a second look.Test: