-
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
Add event_filters
to event triggers in cloudfunctions2 resource
#6278
Add event_filters
to event triggers in cloudfunctions2 resource
#6278
Conversation
The acceptance test for this PR requires access to organization and billing account info - it might require a bit of trial and error in GitHub as I can't do this locally at the moment |
Hello! I am a robot who works on Magic Modules PRs. I've detected that you're a community contributor. @slevenick, a repository maintainer, has been assigned to assist you and help review your changes. ❓ First time contributing? Click here for more detailsYour assigned reviewer will help review your code by:
You can help make sure that review is quick by running local tests and ensuring they're passing in between each push you make to your PR's branch. Also, try to leave a comment with each push you make, as pushes generally don't generate emails. If your reviewer doesn't get back to you within a week after your most recent change, please feel free to leave a comment on the issue asking them to take a look! In the absence of a dedicated review dashboard most maintainers manage their pending reviews through email, and those will sometimes get lost in their inbox. |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 2 files changed, 144 insertions(+)) |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccPrivatecaCertificateAuthority_subordinateCaActivatedByFirstPartyIssuerOnCreation|TestAccPrivatecaCertificateAuthority_privatecaCertificateAuthoritySubordinateExample|TestAccFirebaserulesRelease_BasicRelease|TestAccSqlUser_mysqlDisabled|TestAccComputeInstance_soleTenantNodeAffinities|TestAccCloudfunctions2function_cloudfunctions2FullGcsExample |
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 2 files changed, 143 insertions(+)) |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccPrivatecaCertificateAuthority_subordinateCaActivatedByFirstPartyIssuerOnCreation|TestAccPrivatecaCertificateAuthority_privatecaCertificateAuthoritySubordinateExample|TestAccComputeInstance_soleTenantNodeAffinities|TestAccCloudfunctions2function_cloudfunctions2FullGcsExample|TestAccBigtableAppProfile_bigtableAppProfileMulticlusterExample |
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
Looks like APIs need to be activated in the new project made by the acc test - Need to use trial and error via this PR to get the acceptance test working |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 2 files changed, 162 insertions(+)) |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccFirebaserulesRelease_BasicRelease|TestAccComputeInstance_soleTenantNodeAffinities|TestAccCloudfunctions2function_cloudfunctions2FullGcsExample|TestAccPrivatecaCertificateAuthority_subordinateCaActivatedByFirstPartyIssuerOnCreation|TestAccPrivatecaCertificateAuthority_privatecaCertificateAuthoritySubordinateExample |
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 2 files changed, 162 insertions(+)) |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccComputeInstance_soleTenantNodeAffinities|TestAccCloudfunctions2function_cloudfunctions2FullGcsExample|TestAccPrivatecaCertificateAuthority_subordinateCaActivatedByFirstPartyIssuerOnCreation|TestAccPrivatecaCertificateAuthority_privatecaCertificateAuthoritySubordinateExample |
Tests failed during RECORDING mode: Please fix these to complete your PR |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 2 files changed, 178 insertions(+)) |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccFirebaserulesRelease_BasicRelease|TestAccComputeInstance_soleTenantNodeAffinities|TestAccCloudfunctions2function_cloudfunctions2FullGcsExample|TestAccSqlUser_mysqlDisabled|TestAccPrivatecaCertificateAuthority_privatecaCertificateAuthoritySubordinateExample|TestAccPrivatecaCertificateAuthority_subordinateCaActivatedByFirstPartyIssuerOnCreation |
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
8e4841e
to
5dbe61e
Compare
…` acceptance test
… made by Eventarc
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 2 files changed, 144 insertions(+)) |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccComputeInstance_soleTenantNodeAffinities|TestAccCloudfunctions2function_cloudfunctions2FullGcsExample|TestAccActiveDirectoryDomain_activeDirectoryDomainBasicExample|TestAccActiveDirectoryPeering_activeDirectoryPeeringBasicExample|TestAccActiveDirectoryDomain_update|TestAccPrivatecaCertificateAuthority_subordinateCaActivatedByFirstPartyIssuerOnCreation|TestAccPrivatecaCertificateAuthority_privatecaCertificateAuthoritySubordinateExample|TestAccContainerCluster_withMeshCertificatesConfig |
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
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.
Awesome! Just a few minor things and then I think we're good to go! Thanks!
Currently, only a subset of attributes are supported for filtering. Use the `gcloud eventarc providers describe` command to learn more about events and their attributes. | ||
Do not filter for the 'type' attribute here, as this is already achieved by the resource's `event_type` attribute. | ||
- !ruby/object:Api::Type::String | ||
name: value |
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.
same, required: true
. we're also able to check if these were updatable? just curious if we want to add input: true
here too? we could maybe add an update test if we wanted to test it - or just run a quick update test to see if it 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.
I manually tested changing the value
(as there are not other possible values of attribute
in the context of a GCS event) and it can change ok 👍
I could imagine an argument that a cloud function being triggered in a different way is a fundamentally 'different' cloud function than the old version, so a ForceNew and replacing the whole set of resources that make up the function + its trigger could makes sense. My gut feeling is to not do input:true
because the 1 resource that needs changing can be updated, but I'm not strongly holding onto that belief
mmv1/templates/terraform/examples/cloudfunctions2_full_gcs.tf.erb
Outdated
Show resolved
Hide resolved
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 2 files changed, 266 insertions(+)) |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccComputeInstance_soleTenantNodeAffinities|TestAccCloudfunctions2function_cloudfunctions2BasicAuditlogsExample|TestAccCloudfunctions2function_cloudfunctions2BasicGcsExample|TestAccActiveDirectoryDomain_update|TestAccActiveDirectoryDomain_activeDirectoryDomainBasicExample|TestAccActiveDirectoryPeering_activeDirectoryPeeringBasicExample|TestAccPrivatecaCertificateAuthority_privatecaCertificateAuthoritySubordinateExample |
@megan07 I'm glad you asked about Permadiff terminal output
I'll get on with this next week 😅 Edit on Monday: Addressed the permadiff and pushed the new test that surfaced it in fb04576 |
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 2 files changed, 266 insertions(+)) |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccFirebaserulesRelease_BasicRelease|TestAccSqlUser_mysqlDisabled|TestAccCloudRunService_cloudRunServiceStaticOutboundExample|TestAccComputeInstance_soleTenantNodeAffinities|TestAccActiveDirectoryPeering_activeDirectoryPeeringBasicExample|TestAccActiveDirectoryDomain_update|TestAccActiveDirectoryDomain_activeDirectoryDomainBasicExample|TestAccCloudfunctions2function_cloudfunctions2BasicAuditlogsExample|TestAccCloudfunctions2function_cloudfunctions2BasicGcsExample|TestAccCloudFunctions2Function_fullUpdate|TestAccCGCSnippet_eventarcWorkflowsExample|TestAccPrivatecaCertificateAuthority_privatecaCertificateAuthoritySubordinateExample |
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
Make code homogenous between the two tests to faciliatate this
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 2 files changed, 266 insertions(+)) |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccComputeInstance_soleTenantNodeAffinities|TestAccCloudfunctions2function_cloudfunctions2BasicAuditlogsExample|TestAccCloudFunctions2Function_fullUpdate|TestAccCGCSnippet_eventarcWorkflowsExample|TestAccActiveDirectoryPeering_activeDirectoryPeeringBasicExample|TestAccActiveDirectoryDomain_update|TestAccActiveDirectoryDomain_activeDirectoryDomainBasicExample|TestAccCloudfunctions2function_cloudfunctions2BasicGcsExample|TestAccCloudRunService_cloudRunServiceStaticOutboundExample|TestAccPrivatecaCertificateAuthority_privatecaCertificateAuthoritySubordinateExample |
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
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.
LGTM! Thanks!!
…ogleCloudPlatform#6278) * Add `eventFilters` in cloudfunctions2 API definition * Remove trailing whitespace * Start adding cloudfunctions2 eventarc acceptance test that uses GCS * Add missing IAM bindings to prevent API errors during `FullGcsExample` acceptance test * Make `eventTrigger.pubsubTopic` default from the API to handle topics made by Eventarc * Update field description for `attribute` - info to learn more about possible values * Remove unneeded attribute from `google_storage_bucket_object` * Add API service enabling to new project in test, fix provider used to make project * Fix inbuilt function call in acceptance test * Add explicit dependencies to try make API calls happen after the API is turned on * Remove generated acceptance test and add handwritten test with 2 stages * Remove `depends_on` attributes from example config * Fix bug in test's TF config, make project ID and name the same * Replace use of `for_each` in acceptance test There is an open issue about how for_each loops don't work : hashicorp/terraform-plugin-sdk#536 * Add `run.googleapis.com` to project * Change `disable_dependent_services` to `false` for eventarc API in acceptance tests Change example config to match, too * Enable `artifactregistry.googleapis.com` in acceptance test & example * Add missing IAM binding to cloud function service account - `roles/artifactregistry.reader` Odd that this is needed, as it wasn't required when I created a similar cloud function in an existing GCP project (where APIs were already activated etc) * Add explicit dependency between cloud function resource and IAM bindings for its service account Address "Unable to retrieve the repository metadata" error that suggests service account doesn't have permissions in place * Enable `cloudbuild.googleapis.com` API in acceptance test and example * Enable `pubsub.googleapis.com` API in acceptance test and example * Fix test so step 1 of test enables all the APIs * Replace zipped cloud function code with zip made by `archive_file` Terraform resource * Add repo-level IAM binding (in addition to project level) using inferred repo ID * Fix missing quotation mark in test :( * Split project into 3 stages and remove repo-specific IAM binding This is after running the test locally and it succeeding in 3 steps but not in 2 steps * Fix problem where TF couldn't delete resources at end of test Couldn't delete the cloud function because the API had been disabled * Update test to get project ID from ENV instead of project created by the test Update test and example block with needed variables Remove handwritten test * Fix API spec to make required fields required * Update event_filter fields' descriptions for clarity * Change GCS event_filter test to basic as it can't use all fields * Add example test that shows the operator field in use * Fix whitespace * Add test for updating `event_filters` blocks * Reuse generated cloudfunction2 test functions in handwritten test Make code homogenous between the two tests to faciliatate this
Description
Closes hashicorp/terraform-provider-google#12021
This PR adds the ability to set event filters within event triggers defined when creating a 2nd generation cloud function. Previously users could make 2nd gen functions be triggered by events sent from an 'event provider' (i.e. a Google API) within Eventarc, but now they can filter those events based on the event's attributes.
Valid attributes that can be used in filters differ by the event provider - here's the relevant part of the docs that describes a gcloud command to learn more about the events and attributes for a given event provider.
E.g. All events related to the
storage.googleapis.com
event provider have attributestype
andbucket
- and bucket wouldn't be an attribute on non-GCS eventsThings to note
How should- addressed in fb04576 - docs are here and path patterns aren't appropriate for all attributesoperator
be used? I couldn't work it out for an acceptance testThe
type
attribute cannot be used in the newevent_filters
block because it is already set by theevent_type
field inside theevent_trigger
block.A configuration similar to the one below causes this error
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)