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 exclusions option for logging sink resources #7335

Merged
merged 5 commits into from
Oct 9, 2020

Conversation

pmoncadaisla
Copy link
Contributor

Added as defined in https://cloud.google.com/logging/docs/reference/v2/rest/v2/projects.sinks#LogExclusion

Log entries that match any of the exclusion filters will not be exported.
If a log entry is matched by both filters and one of exclusion_filters it will not be exported.

Affects to the following resources:

  • google_logging_billing_sink
  • google_logging_project_sink
  • google_logging_organization_sink

Added as definedd in https://cloud.google.com/logging/docs/reference/v2/rest/v2/projects.sinks#LogExclusion
Log entries that match any of the exclusion filters will not be exported.
If a log entry is matched by both filter and one of exclusion_filters it will not be exported.
Copy link
Collaborator

@slevenick slevenick left a comment

Choose a reason for hiding this comment

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

Thanks for adding this! I'm running the tests, but is there a reason to have disabled be a string rather than a bool?

google/resource_logging_sink.go Outdated Show resolved Hide resolved
This will avoid unnecessary conversions and it is also defined
as a boolean in the API client.
@pmoncadaisla
Copy link
Contributor Author

Having disabled as TypeString didn't make sense. I have changed it to TypeBool.
Thanks for the catch @slevenick !

unique_writer_identity = false
}

resource "google_logging_project_bucket_config" "logbucket" {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like this needs a project field defined.

Getting this error: on config992620966/terraform_plugin_test.tf line 21, in resource "google_logging_project_bucket_config" "logbucket": 21: resource "google_logging_project_bucket_config" "logbucket" { The argument "project" is required, but no definition was found. --- FAIL: TestAccLoggingProjectSink_loggingbucket (2.32s)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for this! I was running tests incorrectly locally and I thought they ran in CI. I have fixed this issue and a couple issues more.

stAccLoggingProjectSink_loggingbucket'
==> Checking source code against gofmt...
==> Checking that code complies with gofmt requirements...
go generate  ./...
TF_ACC=1 TF_SCHEMA_PANIC_ON_ERROR=1 go test ./google -v -run=TestAccLoggingProjectSink_loggingbucket -timeout 240m -ldflags="-X=github.com/hashicorp/terraform-provider-google/version.ProviderVersion=acc"
=== RUN   TestAccLoggingProjectSink_loggingbucket
=== PAUSE TestAccLoggingProjectSink_loggingbucket
=== CONT  TestAccLoggingProjectSink_loggingbucket
--- PASS: TestAccLoggingProjectSink_loggingbucket (9.01s)
PASS
ok  	github.com/hashicorp/terraform-provider-google/google	9.955s

This test resource does not  createa a logging bucket, it just
modifies its config, therefore it is not suitable for the test.

For this test, the sink with exclusions will be tested directly with
the _Default log bucket as we want to test the exclusions, which is enough.
@pmoncadaisla
Copy link
Contributor Author

I have fixed an issue with tests, where unnecessary google_logging_project_bucket_config was created wrongly.
Also fixed some old bool conversions and removed Computed field from exclusions as it does not need to be computed.

@slevenick slevenick self-requested a review October 2, 2020 21:00
Copy link
Collaborator

@slevenick slevenick left a comment

Choose a reason for hiding this comment

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

This looks great! Can you add documentation for these fields to the relevant resources?

For example here: https://github.com/hashicorp/terraform-provider-google/blob/master/website/docs/r/logging_project_sink.html.markdown

@ghost ghost added size/l and removed size/m labels Oct 6, 2020
@pmoncadaisla pmoncadaisla requested a review from slevenick October 6, 2020 07:24
@pmoncadaisla
Copy link
Contributor Author

Awesome, thanks for the catch again.

I have added documentation with an example.

Copy link
Collaborator

@slevenick slevenick left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution!

@ghost
Copy link

ghost commented Nov 9, 2020

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 👉 [email protected]. Thanks!

@ghost ghost locked as resolved and limited conversation to collaborators Nov 9, 2020
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.

2 participants