Skip to content
This repository has been archived by the owner on Jun 4, 2021. It is now read-only.

Gitlabbinding #1177

Merged
merged 1 commit into from
May 8, 2020
Merged

Gitlabbinding #1177

merged 1 commit into from
May 8, 2020

Conversation

tzununbekov
Copy link
Member

Fixes #1149

Basically, a shameless copy of GitHubBinding

Release Note

GitLabBinding introduced

@knative-prow-robot knative-prow-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Apr 28, 2020
@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Apr 28, 2020
@knative-prow-robot knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 28, 2020
@lgtm-com
Copy link

lgtm-com bot commented Apr 28, 2020

This pull request introduces 1 alert and fixes 1 when merging 39dd9cd into 420d1fa - view on LGTM.com

new alerts:

  • 1 for Useless assignment to local variable

fixed alerts:

  • 1 for Useless assignment to local variable

@tzununbekov
Copy link
Member Author

/hold

@knative-prow-robot knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 28, 2020
@lgtm-com
Copy link

lgtm-com bot commented Apr 28, 2020

This pull request introduces 1 alert and fixes 1 when merging a83b785 into 9aa4097 - view on LGTM.com

new alerts:

  • 1 for Useless assignment to local variable

fixed alerts:

  • 1 for Useless assignment to local variable

@knative-prow-robot knative-prow-robot added area/test-and-release and removed approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Apr 29, 2020
@tzununbekov
Copy link
Member Author

/unhold

@knative-prow-robot knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 29, 2020
@tzununbekov
Copy link
Member Author

/retest

@lgtm-com
Copy link

lgtm-com bot commented Apr 30, 2020

This pull request fixes 1 alert when merging 7313ac0 into c927926 - view on LGTM.com

fixed alerts:

  • 1 for Useless assignment to local variable

@lberk
Copy link
Member

lberk commented Apr 30, 2020

/assign

lberk
lberk reviewed Apr 30, 2020
View reviewed changes
gitlab/cmd/webhook/main.go Outdated Show resolved Hide resolved
lberk
lberk reviewed Apr 30, 2020
View reviewed changes
gitlab/cmd/webhook/main.go

func main() {
ctx := webhook.WithOptions(signals.NewContext(), webhook.Options{
ServiceName: "gitlab-source-webhook",
Copy link
Member

Choose a reason for hiding this comment

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

why not logconfig.WebhookName() and ensure we match the name passed to sharedmain.WebhookMainWithContext ?

Copy link
Member

Choose a reason for hiding this comment

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

aha, you've named it gitlab-source-webhook in the service yaml, gotcha 👍

mattmoor
mattmoor reviewed Apr 30, 2020
View reviewed changes
gitlab/client.go
}

// New instantiates a new gitlab client from the access token from the GitLabBinding
func New(ctx context.Context) (*gitlab.Client, error) {
Copy link
Member

Choose a reason for hiding this comment

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

This is awesome! One realization I had this morning (and was chatting with @lberk about) was aligning the changes we make to the environment with existing tooling. e.g. in the case of GitHubBinding, I think we should support the hub CLI. Not sure if there is a Gitlab equivalent, or if it's auth is flexible enough to just trivially add this, but here's the Github issue: #1185

@lberk
Copy link
Member

lberk commented Apr 30, 2020 •
edited
Loading

/lgtm
outside the small comment nits this seems logical to me -- after that will get the approve tag (assuming @mattmoor doesn't have any additional comments)

@knative-prow-robot knative-prow-robot added lgtm Indicates that a PR is ready to be merged. and removed lgtm Indicates that a PR is ready to be merged. labels Apr 30, 2020
@lgtm-com
Copy link

lgtm-com bot commented May 6, 2020

This pull request fixes 1 alert when merging 82e0bef into 23d4d5a - view on LGTM.com

fixed alerts:

  • 1 for Useless assignment to local variable

@knative-metrics-robot
Copy link

The following is the coverage report on the affected files.
Say /test pull-knative-eventing-contrib-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
gitlab/pkg/apis/bindings/v1alpha1/gitlabbinding_defaults.go Do not exist 100.0%
gitlab/pkg/apis/bindings/v1alpha1/gitlabbinding_lifecycle.go Do not exist 90.6%
gitlab/pkg/apis/bindings/v1alpha1/gitlabbinding_validation.go Do not exist 100.0%
gitlab/pkg/apis/bindings/v1alpha1/register.go Do not exist 100.0%

@lgtm-com
Copy link

lgtm-com bot commented May 6, 2020

This pull request fixes 1 alert when merging 9886ba3 into 23d4d5a - view on LGTM.com

fixed alerts:

  • 1 for Useless assignment to local variable

@tzununbekov
Copy link
Member Author

/retest

@lberk
Copy link
Member

lberk commented May 6, 2020

/lgtm
/approve
(I'm not sure what's going on with e2e testing atm)

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label May 6, 2020
@tzununbekov
Copy link
Member Author

/assign @chaodaiG

@chaodaiG
Copy link
Contributor

chaodaiG commented May 7, 2020

/approve

@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chaodaiG, lberk, tzununbekov

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@knative-prow-robot knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 7, 2020
@matzew
Copy link
Member

matzew commented May 8, 2020

/retest

@knative-prow-robot knative-prow-robot merged commit 1d75551 into knative:master May 8, 2020
@knative-prow-robot
Copy link
Contributor

@tzununbekov: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
pull-knative-eventing-contrib-integration-tests 9886ba3 link /test pull-knative-eventing-contrib-integration-tests

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

This was referenced May 19, 2020
Merged
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mattmoor mattmoor mattmoor left review comments

@lberk lberk lberk left review comments

@n3wscott n3wscott Awaiting requested review from n3wscott

@sebgoa sebgoa Awaiting requested review from sebgoa

Assignees

@lberk lberk

@chaodaiG chaodaiG

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/test-and-release cla: yes Indicates the PR's author has signed the CLA. lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Create a GitLabBinding
8 participants
@tzununbekov @lberk @knative-metrics-robot @chaodaiG @knative-prow-robot @matzew @mattmoor @googlebot