Skip to content
This repository was archived by the owner on Mar 11, 2022. It is now read-only.

handler: add Ref check to WebhookHandler#293

Merged
seanmalloy merged 2 commits intomasterfrom
issue-252
Feb 18, 2020
Merged

handler: add Ref check to WebhookHandler#293
seanmalloy merged 2 commits intomasterfrom
issue-252

Conversation

@rhalat
Copy link
Copy Markdown
Contributor

@rhalat rhalat commented Feb 11, 2020

Description

GitHub webhook is triggered on all Push events and without checking
branch in WebhookHandler new job is created every time there is push to
repo, no matter what branch. In this PR I added check, so job is created
only when URI and Ref matches in push event and GitOpsConfig CR.

Additionally it's fixing Travis CI that was failing on hello-world-yaml-cr1 test by increasing timeout.

Fixes #252

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Checklist

  • Unit tests and e2e tests updated
  • Documentation updated

Comment thread pkg/handler/handler.go Outdated
Comment thread pkg/handler/handler.go
@seanmalloy seanmalloy added this to the v0.1.3 milestone Feb 11, 2020
@seanmalloy
Copy link
Copy Markdown
Contributor

@rhalat what if .Spec.ParameterSource.Ref or .Spec.TemplateSource.Ref is not a branch? How does this code handle a git tag or git sha1?

@codecov
Copy link
Copy Markdown

codecov bot commented Feb 12, 2020

Codecov Report

Merging #293 into master will increase coverage by 5.55%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #293      +/-   ##
=========================================
+ Coverage    0.00%   5.55%   +5.55%     
=========================================
  Files           1       2       +1     
  Lines          52     108      +56     
=========================================
+ Hits            0       6       +6     
- Misses         52     102      +50     
Impacted Files Coverage Δ
pkg/handler/handler.go 10.71% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 62b9273...ec0e320. Read the comment docs.

@rhalat
Copy link
Copy Markdown
Contributor Author

rhalat commented Feb 12, 2020

@seanmalloy is it supposed to handle something different than branches? When it comes to sha1, I believe it won't trigger job and when it comes to tags we can think of it and I can include it in unit tests, but what in case where branch and tag have same names? In GitOpsConfig CR we are not distinguish them.

@akavel
Copy link
Copy Markdown

akavel commented Feb 12, 2020

@seanmalloy @rhalat Maybe we could just modify README telling users to set Ref to refs/tags/foobar when they want to trigger on foobar tag? I suppose this probably already works now with this PR, and would also help us avoid name conflicts?

@seanmalloy
Copy link
Copy Markdown
Contributor

@rhalat please make the code work with git branches and git tags. We can worry about the git SHA1 use case at some point in the future.

As @akavel mentioned maybe we just need to update the docs for now, so people know how to use the webhook trigger with both branches and tags.

@rhalat
Copy link
Copy Markdown
Contributor Author

rhalat commented Feb 13, 2020

I updated README.md and I'm close to finish with unit tests.

Comment thread pkg/handler/handler_test.go Outdated
Comment thread pkg/handler/handler_test.go Outdated
Comment thread pkg/handler/handler.go Outdated
@akavel
Copy link
Copy Markdown

akavel commented Feb 18, 2020

@rhalat Please add an extra commit increasing the timeout in e2e-test.sh for hello-world-yaml-cr1. Please make sure the commit message explains the reasons: from CI logs, we see that one run barely completes in time:

https://travis-ci.com/KohlsTechnology/eunomia/jobs/288138640#L2471

so the failure in the other run will probably disappear if we increase timeout:

https://travis-ci.com/KohlsTechnology/eunomia/jobs/288138641#L2469

As a side note, personally, I hate that the tests are taking so long now; I hope we'll find some way(s) to speed them up at some point.

[optional] It could also be nice to squash some of the commits in this PR now (e.g. git rebase -i, or easier if slower approach with git cherry-pick), so that 2 commits remain:

  1. main feature
  2. the timeout increase.

Robert Halat added 2 commits February 18, 2020 13:11
GitHub webhook is triggered on all Push events and without checking
branch in WebhookHandler new job is created every time there is push to
repo, no matter what branch. In this PR I added check, so job is created
only when URI and Ref matches in push event and GitOpsConfig CR.

Fixes #252
Travis CI is failing on hello-world-yaml-cr1 test from time  to time. 
When it succeeds it barely completes in time, so increasing timeout 
should solve the issue.
Copy link
Copy Markdown

@akavel akavel left a comment

Choose a reason for hiding this comment

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

👍 LGTM

@seanmalloy seanmalloy merged commit 7577042 into master Feb 18, 2020
@seanmalloy seanmalloy deleted the issue-252 branch February 18, 2020 17:27
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Development

Successfully merging this pull request may close these issues.

Webhook code should filter for a ref/branch name match before kicking off

3 participants