CodeBuild Webhook FilterGroups #8110

tomkuehl · 2019-03-28T10:02:32Z

Community Note

Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
Please do not leave "+1" comments, they generate extra noise for pull request followers and do not help prioritize the request

Changes proposed in this pull request:

Add filter_group argument to resource aws_codebuild_webhook

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAccAWSCodeBuildWebhook_FilterGroup'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -parallel 20 -run=TestAccAWSCodeBuildWebhook_FilterGroup -timeout 120m
?       github.com/terraform-providers/terraform-provider-aws   [no test files]
=== RUN   TestAccAWSCodeBuildWebhook_FilterGroup
=== PAUSE TestAccAWSCodeBuildWebhook_FilterGroup
=== CONT  TestAccAWSCodeBuildWebhook_FilterGroup
--- PASS: TestAccAWSCodeBuildWebhook_FilterGroup (42.81s)
PASS
ok      github.com/terraform-providers/terraform-provider-aws/aws       42.871s

ToDo:

Fix update of existing hook
Update test
Update docs

tomkuehl · 2019-03-28T10:22:11Z

Shall i add a notice that the branch_filter argument is deprecated?

pedrofaria · 2019-04-12T08:17:24Z

Shall i add a notice that the branch_filter argument is deprecated?

IMHO, no.

This option still available on Go SDK.

tomkuehl · 2019-04-15T13:16:27Z

I added a note about using filter_group over branch_filter like in the AWS docs: https://docs.aws.amazon.com/de_de/codebuild/latest/APIReference/API_Webhook.html

It's working so far. Only thing left are the tests. First I have to read the docs and then I will implement them 😅

tomkuehl · 2019-04-15T15:07:18Z

I added the test as far as i came. It throws this exception: OAuthProviderException: Repository not found or permission denied. Where can i edit the repo it uses? Don't find it.. Can someone with a little bit more experience help me out?

bflad · 2019-04-15T15:12:44Z

Hi @tomkuehl 👋 The BitBucket and GitHub repositories the CodeBuild resources use for testing can be controlled via the AWS_CODEBUILD_BITBUCKET_SOURCE_LOCATION and AWS_CODEBUILD_GITHUB_SOURCE_LOCATION environments variables:

https://github.com/terraform-providers/terraform-provider-aws/blob/f329cc925d4b679a1de120a2a28bce48a14a8e2a/aws/resource_aws_codebuild_project_test.go#L16-L38

tomkuehl · 2019-04-15T16:11:10Z

Thank you @bflad !! Now the test works. I only have one question left: Why? :D I thought these numbers (e.g. 3033132000: https://github.com/terraform-providers/terraform-provider-aws/pull/8110/files#diff-8984e55d6f33639a7b185d987d2233eeR174) where auto-generated?

And what exactly is this step for? https://github.com/terraform-providers/terraform-provider-aws/blob/master/aws/resource_aws_codebuild_webhook_test.go#L149-L154 Do we need it in the filter_group test as well? Because it doesn't work.. ^^

bflad · 2019-04-15T16:50:16Z

Those TypeSet hashes (e.g. 3033132000) can either be tested like that, which can be brittle depending on the configuration, or by inspecting the &webhook returned by testAccCheckAWSCodeBuildWebhookExists, e.g.

// After testAccCheckAWSCodeBuildWebhookExists() call in Check
testAccCheckAWSCodeBuildWebhookFilters(&webhook, [][]*codebuild.WebhookFilter{
  // fill in details
}),

// new top level function
func testAccCheckAWSCodeBuildWebhookFilters(webhook *codebuild.Webhook, expectedFilters [][]*codebuild.WebhookFilter) resource.TestCheckFunc {
	return func(s *terraform.State) error {
		if webhook == nil {
			return fmt.Errorf("webhook missing")
		}

		if !reflect.DeepEqual(webhook.FilterGroups, expectedFilters) {
			return fmt.Errorf("expected webhook filter configuration (%v), got: %v", expectedFilters, webhook.FilterGroups)
		}

		return nil
	}
}

And what exactly is this step for? https://github.com/terraform-providers/terraform-provider-aws/blob/master/aws/resource_aws_codebuild_webhook_test.go#L149-L154 Do we need it in the filter_group test as well? Because it doesn't work.. ^^

Yes, that should be included. It verifies that someone can run terraform import successfully on the infrastructure and not show any differences in Terraform immediately after. If it is failing, it likely means the resource Read() function is not properly calling d.Set() for the attribute. If you provide the error output from the testing, we can provide more targeted guidance.

tomkuehl · 2019-04-15T19:55:11Z

Thanks!

This is the error from the test:

ImportStateVerify attributes not equivalent. Difference is shown below. Top is actual, bottom is expected.

(map[string]string) {}

(map[string]string) (len=7) {
(string) (len=14) "filter_group.#": (string) (len=1) "2",
(string) (len=46) "filter_group.178477371.exclude_matched_pattern": (string) (len=5) "false",
(string) (len=30) "filter_group.178477371.pattern": (string) (len=17) "refs/heads/master",
(string) (len=27) "filter_group.178477371.type": (string) (len=8) "HEAD_REF",
(string) (len=47) "filter_group.2768903781.exclude_matched_pattern": (string) (len=5) "false",
(string) (len=31) "filter_group.2768903781.pattern": (string) (len=20) "PULL_REQUEST_CREATED",
(string) (len=28) "filter_group.2768903781.type": (string) (len=5) "EVENT"
}

And the error from d.Set("filter_group", project.Webhook.FilterGroups) says: filter_group.0: '' expected a map, got 'slice'

danisabad · 2019-04-16T13:17:22Z

Hello, @tomkuehl! Thank you for this PR.
I've just tested it for 0.12, works well.

I can see that there is a functionality to create multiple event filter groups for a webhook. Do you have plans to implement that as well?

tomkuehl · 2019-04-16T14:33:52Z

Hey @danisabad, thanks for testing it!

What exactly do you mean? You can already provide multiple event types as a comma-separated string. e.g.:

filter_group {
    type = "EVENT"
    pattern = "PUSH,PULL_REQUEST_CREATED"
}

danisabad · 2019-04-16T20:48:26Z

Let me illustrate example:

sgal · 2019-05-07T07:36:19Z

@tomkuehl This looks awesome! How far from completion you think this PR is? I would love to have this feature available cause now I have to stick with cloudformation templates which are quite cumbersome to reuse in this case.

tomkuehl · 2019-05-07T09:00:25Z

Hi @sgal! Currently the test is failing & last time i worked on it, i didn't find out why... I also want to implement the use case @danisabad showed. (Didn't know it was possible in the first place). When these 2 things are done it should be ready to merge. Currently i don't have much time to work on this, so any help is welcome :)

raivenlilianto · 2019-05-09T08:24:17Z

Hi @tomkuehl, Thank you for this PR!
I already fix acceptance test, Unfortunately I can't push directly into your fork.
See attachment.

Fix output Filter Groups (need to flatten the output)
Fix acceptance test.

Hope this help 👍

resource_aws_codebuild_webhook.zip

Edit

$ make testacc TESTARGS='-run=TestAccAWSCodeBuildWebhook_FilterGroup'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -parallel 20 -run=TestAccAWSCodeBuildWebhook_FilterGroup -timeout 120m
?   	github.com/terraform-providers/terraform-provider-aws	[no test files]
=== RUN   TestAccAWSCodeBuildWebhook_FilterGroup
=== PAUSE TestAccAWSCodeBuildWebhook_FilterGroup
=== CONT  TestAccAWSCodeBuildWebhook_FilterGroup
--- PASS: TestAccAWSCodeBuildWebhook_FilterGroup (87.06s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	87.078s

tomkuehl · 2019-05-14T06:37:22Z

Awesome @raivenlilianto, thank you!!!

So if i'm right, the last thing missing is the functionality to add multiple event filter groups? I start working on this now

talton-heb · 2019-05-15T20:39:22Z

Shall i add a notice that the branch_filter argument is deprecated?

IMHO, no.

This option still available on Go SDK.

If you try and set both branchFilter and filterGroups at the same time, AWS gives a warning along the lines of "You cannot use branchFilter and filterGroups at the same time. Specify the branch filter using filterGroups"

Since filterGroups are a strict superset of the branchFilter functionality, I would suggest deprecating the branch_filter terraform config option.

tomkuehl · 2019-05-17T13:01:15Z

Multiple filter_groups are working now... But i broke the tests ☹️ Could someone please take a look at this? When the tests work again this PR is ready to merge IMO

sgal · 2019-05-21T11:53:43Z

@tomkuehl According to CI it is all green now - is there anything that's missing?

tomkuehl · 2019-05-21T14:14:34Z

The CI was red because the test was incompatible with multiple filter groups. This one is fixed now, but if i run make testacc TESTARGS='-run=TestAccAWSCodeBuildWebhook_FilterGroup' it results in an error:

?       github.com/terraform-providers/terraform-provider-aws   [no test files]
=== RUN   TestAccAWSCodeBuildWebhook_FilterGroup
=== PAUSE TestAccAWSCodeBuildWebhook_FilterGroup
=== CONT  TestAccAWSCodeBuildWebhook_FilterGroup
--- FAIL: TestAccAWSCodeBuildWebhook_FilterGroup (41.39s)
    testing.go:538: Step 1 error: ImportStateVerify attributes not equivalent. Difference is shown below. Top is actual, bottom is expected.
        
        (map[string]string) {
        }
        
        
        (map[string]string) (len=12) {
         (string) (len=14) "filter_group.#": (string) (len=1) "2",
         (string) (len=32) "filter_group.1895770492.filter.#": (string) (len=1) "2",
         (string) (len=56) "filter_group.1895770492.filter.0.exclude_matched_pattern": (string) (len=5) "false",
         (string) (len=40) "filter_group.1895770492.filter.0.pattern": (string) (len=4) "PUSH",
         (string) (len=37) "filter_group.1895770492.filter.0.type": (string) (len=5) "EVENT",
         (string) (len=56) "filter_group.1895770492.filter.1.exclude_matched_pattern": (string) (len=4) "true",
         (string) (len=40) "filter_group.1895770492.filter.1.pattern": (string) (len=17) "refs/heads/master",
         (string) (len=37) "filter_group.1895770492.filter.1.type": (string) (len=8) "HEAD_REF",
         (string) (len=31) "filter_group.936864534.filter.#": (string) (len=1) "1",
         (string) (len=55) "filter_group.936864534.filter.0.exclude_matched_pattern": (string) (len=5) "false",
         (string) (len=39) "filter_group.936864534.filter.0.pattern": (string) (len=20) "PULL_REQUEST_UPDATED",
         (string) (len=36) "filter_group.936864534.filter.0.type": (string) (len=5) "EVENT"
        }
        
FAIL
FAIL    github.com/terraform-providers/terraform-provider-aws/aws       41.446s
make: *** [testacc] Error 1

I didn't find out why it happens yet..

tomkuehl · 2019-05-22T15:21:17Z

Here we go, fixed the tests! IMO this PR is now ready to merge :)

tomkuehl · 2019-05-28T09:55:12Z

Can we get this reviewed? :) cc @bflad

KursLabIgor · 2019-05-31T13:03:56Z

Guys, please merge it. I really surprised that I cant manage it from terraform.

ivz-abhinav · 2019-06-04T12:17:42Z

Thanks for the efforts @tomkuehl

I hope this is merged soon, we need it for one of the projects.

bflad

Thanks so much, @tomkuehl, LGTM! 🚀

Output from acceptance testing (failure unrelated):

--- FAIL: TestAccAWSCodeBuildWebhook_Bitbucket (18.23s)
--- PASS: TestAccAWSCodeBuildWebhook_BranchFilter (27.73s)
--- PASS: TestAccAWSCodeBuildWebhook_GitHub (29.55s)
--- PASS: TestAccAWSCodeBuildWebhook_FilterGroup (29.98s)
--- PASS: TestAccAWSCodeBuildWebhook_GitHubEnterprise (35.98s)

bflad · 2019-06-07T12:24:29Z

This has been released in version 2.14.0 of the Terraform AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

ghost · 2019-11-03T15:38:32Z

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. Thanks!

tomkuehl added 2 commits March 28, 2019 09:26

Add filter_group to webhook schema

f1755f6

Update filter_group properties

c92281c

ghost added size/M Managed by automation to categorize the size of a PR. service/codebuild Issues and PRs that pertain to the codebuild service. labels Mar 28, 2019

Update documentation

668f517

ghost added the documentation Introduces or discusses updates to documentation. label Mar 28, 2019

tomkuehl added 2 commits April 15, 2019 15:01

Add note to use filter_group over branch_filter

cf31104

Fix webhook update

f429a75

Add filter_group test

b96d47d

ghost added the tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. label Apr 15, 2019

bflad added the enhancement Requests to existing resources that expand the functionality or scope. label Apr 15, 2019

Update test

ba1fc3e

ghost added size/L Managed by automation to categorize the size of a PR. and removed size/M Managed by automation to categorize the size of a PR. labels Apr 15, 2019

Update test

305f333

Update test & output for webhook

8583516

fix lint

dc8ac1c

tomkuehl added 3 commits May 17, 2019 14:13

Add multiple filter_groups

6487502

Update documentation

60f9e3e

Update tests

05a95a0

Fix flatten

4534a4d

tomkuehl changed the title ~~[WIP] CodeBuild Webhook FilterGroups~~ CodeBuild Webhook FilterGroups May 22, 2019

bflad added this to the v2.14.0 milestone Jun 6, 2019

bflad approved these changes Jun 6, 2019

View reviewed changes

bflad merged commit 3a487a1 into hashicorp:master Jun 6, 2019

bflad added a commit that referenced this pull request Jun 6, 2019

Update CHANGELOG for #8110

fa6ec21

talton-heb mentioned this pull request Jun 12, 2019

Race condition when determining provider for child module data block hashicorp/terraform#21699

Closed

ghost locked and limited conversation to collaborators Nov 3, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CodeBuild Webhook FilterGroups #8110

CodeBuild Webhook FilterGroups #8110

tomkuehl commented Mar 28, 2019 •

edited

Loading

tomkuehl commented Mar 28, 2019

pedrofaria commented Apr 12, 2019

tomkuehl commented Apr 15, 2019

tomkuehl commented Apr 15, 2019

bflad commented Apr 15, 2019

tomkuehl commented Apr 15, 2019

bflad commented Apr 15, 2019

tomkuehl commented Apr 15, 2019

danisabad commented Apr 16, 2019

tomkuehl commented Apr 16, 2019

danisabad commented Apr 16, 2019

sgal commented May 7, 2019

tomkuehl commented May 7, 2019

raivenlilianto commented May 9, 2019 •

edited

Loading

tomkuehl commented May 14, 2019 •

edited

Loading

talton-heb commented May 15, 2019

tomkuehl commented May 17, 2019

sgal commented May 21, 2019

tomkuehl commented May 21, 2019

tomkuehl commented May 22, 2019

tomkuehl commented May 28, 2019

KursLabIgor commented May 31, 2019

ivz-abhinav commented Jun 4, 2019

bflad left a comment

bflad commented Jun 7, 2019

ghost commented Nov 3, 2019

CodeBuild Webhook FilterGroups #8110

CodeBuild Webhook FilterGroups #8110

Conversation

tomkuehl commented Mar 28, 2019 • edited Loading

Community Note

tomkuehl commented Mar 28, 2019

pedrofaria commented Apr 12, 2019

tomkuehl commented Apr 15, 2019

tomkuehl commented Apr 15, 2019

bflad commented Apr 15, 2019

tomkuehl commented Apr 15, 2019

bflad commented Apr 15, 2019

tomkuehl commented Apr 15, 2019

danisabad commented Apr 16, 2019

tomkuehl commented Apr 16, 2019

danisabad commented Apr 16, 2019

sgal commented May 7, 2019

tomkuehl commented May 7, 2019

raivenlilianto commented May 9, 2019 • edited Loading

tomkuehl commented May 14, 2019 • edited Loading

talton-heb commented May 15, 2019

tomkuehl commented May 17, 2019

sgal commented May 21, 2019

tomkuehl commented May 21, 2019

tomkuehl commented May 22, 2019

tomkuehl commented May 28, 2019

KursLabIgor commented May 31, 2019

ivz-abhinav commented Jun 4, 2019

bflad left a comment

Choose a reason for hiding this comment

bflad commented Jun 7, 2019

ghost commented Nov 3, 2019

tomkuehl commented Mar 28, 2019 •

edited

Loading

raivenlilianto commented May 9, 2019 •

edited

Loading

tomkuehl commented May 14, 2019 •

edited

Loading