-
Notifications
You must be signed in to change notification settings - Fork 420
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 BitBucket Interceptor with examples and README #578
Add BitBucket Interceptor with examples and README #578
Conversation
The following is the coverage report on the affected files.
|
61fc8e9
to
b669685
Compare
The following is the coverage report on the affected files.
|
b669685
to
b4e4e56
Compare
The following is the coverage report on the affected files.
|
b4e4e56
to
72651bb
Compare
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
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.
Thanks for adding this @savitaashture I have a few minor comments mostly around organizing the examples and docs. In addition, could you also squash the commits, and mention the issue in the commit message (https://github.com/tektoncd/community/blob/master/standards.md#commit-messages)
// No bitbucket validation required yet. | ||
// if i.Bitbucket != nil { | ||
// | ||
// } |
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.
Don't think this is necessary?
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.
Also, we should add validation for these 😄 (maybe in a future 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.
Can we remove the commented out code?
baeca7b
to
d208426
Compare
The following is the coverage report on the affected files.
|
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.
Almost there, just a few small nits!
// No bitbucket validation required yet. | ||
// if i.Bitbucket != nil { | ||
// | ||
// } |
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.
Can we remove the commented out code?
GitLab: &v1alpha1.GitLabInterceptor{}, | ||
GitHub: &v1alpha1.GitHubInterceptor{}, | ||
GitLab: &v1alpha1.GitLabInterceptor{}, | ||
Bitbucket: &v1alpha1.BitBucketInterceptor{}, |
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 don't think adding it here is doing anything since this is just testing the validation fails with multiple interceptors
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.
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.
hmm, seems ok for now but wondering if we should have a separate test for that...
The following is the coverage report on the affected files.
|
Excellent. Thank you @savitaashture Could you squash up the 2 commits and we can merge! |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dibyom The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
19cdcae
to
c8896b9
Compare
The following is the coverage report on the affected files.
|
/lgtm |
c8896b9
to
33d06b7
Compare
The following is the coverage report on the affected files.
|
33d06b7
to
6bbe4b7
Compare
The following is the coverage report on the affected files.
|
/lgtm |
Changes
Fixes: #362
Submitter Checklist
These are the criteria that every PR should meet, please check them off as you
review them:
See the contribution guide for more details.
Release Notes