8000 Fixed: Codebuild Webhook Filters are to be a list of list of WebhookFilter by JohnPreston · Pull Request #1372 · cloudtools/troposphere · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Fixed: Codebuild Webhook Filters are to be a list of list of WebhookFilter #1372

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Apr 21, 2019
Merged

Fixed: Codebuild Webhook Filters are to be a list of list of WebhookFilter #1372

merged 2 commits into from
Apr 21, 2019

Conversation

JohnPreston
Copy link
Contributor

According to documentation, Web Hooks should be written in the form of List of List of WebHooks
https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-codebuild-project-projecttriggers.html

API call confirms the structure:

"webhook": {
                "url": "https://api.github.com/repos/<OMIT>",
                "payloadUrl": "https://codebuild.eu-west-1.amazonaws.com/webhooks?t=<OMIT>",
                "filterGroups": [
                    [
                        {
                            "type": "EVENT",
                            "pattern": "PULL_REQUEST_CREATED, PULL_REQUEST_UPDATED, PULL_REQUEST_REOPENED",
                            "excludeMatchedPattern": false
                        },
                        {
                            "type": "ACTOR_ACCOUNT_ID",
                            "pattern": "ausername",
                            "excludeMatchedPattern": false
                        }
                    ],
                    [
                        {
                            "type": "EVENT",
                            "pattern": "PULL_REQUEST_CREATED, PULL_REQUEST_UPDATED, PULL_REQUEST_REOPENED",
                            "excludeMatchedPattern": false
                        },
                        {
                            "type": "BASE_REF",
                            "pattern": "ref/heads/master$",
                            "excludeMatchedPattern": false
                        },
                        {
                            "type": "HEAD_REF",
                            "pattern": "ref/heads/branch1$",
                            "excludeMatchedPattern": false
                        }
                    ]
                ]
            },

Hope I used the class appropriately.

@JohnPreston
Fixed: Codebuild Webhook Filters are to be a list of list of WebhookF…
eccd77f 
…ilter

According to documentation, Web Hooks should be written in the form of List of List of WebHooks
https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-codebuild-project-projecttriggers.html

Fixed: Max line length

Fixed: Blank Line
@JohnPreston
Copy link
Contributor Author

Force-pushed to squash 2 commits which were to fix pep-8 issues.

@markpeek
Copy link
Member

Oh interesting. list of lists...likely what you want to do to implement is not to override __init__() but create a validate() function instead and a test case to verify it. Something like this (untested):

def validate(self):
    if 'FilterGroups' not in self.properties:
        return
    for filter_list in self.properties.get('FilterGroups'):
        if not isinstance(filter_list, list):
            raise ....
        for webhook in filterlist:
            if not isinstance(webhook, WebhookFilter):
                raise ...

@JohnPreston
Copy link
Contributor Author

Force-pushed to squash 2 commits which were to fix pep-8 issues.

@markpeek markpeek merged commit 737277b into cloudtools:master Apr 21, 2019
@markpeek
Copy link
Member

Thanks! I did see an issue with counti not getting incremented so I switched to using enumeration and added some tests. Please take a look at 8259f0a to verify.

@phobologic
Copy link
Member

@JohnPreston @markpeek - sorry to revive this, but we just ran into an issue with this.

https://github.com/cloudtools/troposphere/blame/66cea0002c55c3f667ace816ce4c8f457940bea2/troposphere/codebuild.py#L244

Effectively that makes FilterGroups required, but

https://github.com/cloudtools/troposphere/blame/66cea0002c55c3f667ace816ce4c8f457940bea2/troposphere/codebuild.py#L240

has it required as False, as the documentation dictates. Was this intended? We can work around it by setting FilterGroups to a blank list, I think, but I don't believe this is required (it didn't used to be, and the docs don't list it as required). Thanks!

@markpeek
Copy link
Member

Agreed. Looks like ProjectTriggers is not required and Webhook/FilterGroups are not required. Likely that raise line gets changed to a return. Feel free to PR and adding a test is always encouraged. :-)

@phobologic
Copy link
Member

Cool, I'll get to a PR probably tonight.

@phobologic
Copy link
Member

Yeah, we'll need a PR - my workaround (an empty list of lists) fails:

Failed to call UpdateWebhook, reason: Filter group cannot be empty (Service: AWSCodeBuild; Status Code: 400; Error Code: InvalidInputException; Request ID: a9f097fb-8c9a-11e9-9c26-5d3f240d6956)

phobologic added a commit that referenced this pull request Jun 12, 2019
@phobologic
Fix bug where FilterGroups were required, when technically they are not
1091143 
Fixes a bug introduced here #1372

Basically while FilterGroups aren't required, the validate() function
was effectively making them required.  This makes it so validate only
validates FilterGroups if they're actually set.
@phobologic phobologic mentioned this pull request Jun 12, 2019
Merged
markpeek pushed a commit that referenced this pull request Jun 12, 2019
@phobologic @markpeek
Fix bug where FilterGroups were required, when technically they are n…
cd0d621 
…ot (#1424)

Fixes a bug introduced here #1372

Basically while FilterGroups aren't required, the validate() function
was effectively making them required.  This makes it so validate only
validates FilterGroups if they're actually set.
davemasino pushed a commit to davemasino/troposphere that referenced this pull request Oct 17, 2019
@JohnPreston
Fixed: Codebuild Webhook Filters are to be a list of list of WebhookF…
e260b3c 
…ilter (cloudtools#1372)

* Fixed: Codebuild Webhook Filters are to be a list of list of WebhookFilter
According to documentation, Web Hooks should be written in the form of List of List of WebHooks
https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-codebuild-project-projecttriggers.html
davemasino pushed a commit to davemasino/troposphere that referenced this pull request Oct 17, 2019
@phobologic
Fix bug where FilterGroups were required, when technically they are n…
9acf7e0 
…ot (cloudtools#1424)

Fixes a bug introduced here cloudtools#1372

Basically while FilterGroups aren't required, the validate() function
was effectively making them required.  This makes it so validate only
validates FilterGroups if they're actually set.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@JohnPreston @markpeek @phobologic
0