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

add gitlab source to contrib #382

Closed
wants to merge 2 commits into from
Closed

add gitlab source to contrib #382

wants to merge 2 commits into from

Conversation

vincent-pli
Copy link

Proposed Changes

Add gitlab source to contrib/

Add new crd: GitLabSource and related Controller

@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Apr 30, 2019
@knative-prow-robot knative-prow-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Apr 30, 2019
@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: vincent-pli
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: evankanderson

If they are not already assigned, you can assign the PR to them by writing /assign @evankanderson in a comment when ready.

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
Copy link
Contributor

Hi @vincent-pli. Thanks for your PR.

I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@knative-prow-robot knative-prow-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Apr 30, 2019
@vincent-pli vincent-pli force-pushed the add-gitlab-source-to-contrib branch 2 times, most recently from dd96050 to 725af5f Compare April 30, 2019 06:18
@zxDiscovery
Copy link

/ok-to-test

@knative-prow-robot knative-prow-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 30, 2019
@vincent-pli
Copy link
Author

vincent-pli commented May 6, 2019 •
edited
Loading

/assign @evankanderson

@vincent-pli vincent-pli force-pushed the add-gitlab-source-to-contrib branch from 725af5f to 48f7a4d Compare May 6, 2019 02:31
@vincent-pli
Copy link
Author

/assign @evankanderson

@vincent-pli
Copy link
Author

/assign @n3wscott

@vincent-pli vincent-pli force-pushed the add-gitlab-source-to-contrib branch from 48f7a4d to 81397f6 Compare May 10, 2019 07:20
@vincent-pli vincent-pli force-pushed the add-gitlab-source-to-contrib branch from 81397f6 to a3dc5d6 Compare May 10, 2019 07:40
srvaroa
srvaroa reviewed May 13, 2019
View reviewed changes
contrib/gitlab/cmd/controller/main.go Outdated
logCfg := zap.NewProductionConfig()
logCfg.EncoderConfig.EncodeTime = zapcore.ISO8601TimeEncoder
logger, err := logCfg.Build()
logger = logger.With(zap.String(logkey.ControllerType, "gitlab-controller"))
Copy link
Contributor

Choose a reason for hiding this comment

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

This one should be after the error check, to avoid using the instance if errored.

Copy link
Author

Choose a reason for hiding this comment

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

Thank, will fix it soon

@vincent-pli vincent-pli force-pushed the add-gitlab-source-to-contrib branch from a3dc5d6 to 8c1b5c0 Compare May 13, 2019 08:37
mattmoor-sockpuppet
mattmoor-sockpuppet reviewed May 13, 2019
View reviewed changes
contrib/gitlab/cmd/receive_adapter/main.go Outdated
@@ -0,0 +1,92 @@
/*
Copyright 2018 The Knative Authors
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Copyright 2018 The Knative Authors
Copyright 2019 The Knative Authors

contrib/gitlab/pkg/adapter/adapter.go Outdated
@@ -0,0 +1,144 @@
/*
Copyright 2018 The Knative Authors
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Copyright 2018 The Knative Authors
Copyright 2019 The Knative Authors

contrib/gitlab/pkg/adapter/adapter_test.go Outdated
@@ -0,0 +1,382 @@
/*
Copyright 2018 The Knative Authors
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Copyright 2018 The Knative Authors
Copyright 2019 The Knative Authors

contrib/gitlab/pkg/apis/sources/v1alpha1/gitlab_types.go Outdated
@@ -0,0 +1,188 @@
/*
Copyright 2018 The Knative Authors
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Copyright 2018 The Knative Authors
Copyright 2019 The Knative Authors

contrib/gitlab/pkg/apis/sources/v1alpha1/gitlab_types_test.go Outdated
@@ -0,0 +1,268 @@
/*
Copyright 2018 The Knative Authors
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Copyright 2018 The Knative Authors
Copyright 2019 The Knative Authors

contrib/gitlab/pkg/reconciler/doc.go Outdated
@@ -0,0 +1,18 @@
/*
Copyright 2018 The Knative Authors
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Copyright 2018 The Knative Authors
Copyright 2019 The Knative Authors

contrib/gitlab/pkg/reconciler/gitlabsource.go Outdated
@@ -0,0 +1,384 @@
/*
Copyright 2018 The Knative Authors
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Copyright 2018 The Knative Authors
Copyright 2019 The Knative Authors

contrib/gitlab/pkg/reconciler/gitlabsource_test.go Outdated
@@ -0,0 +1,769 @@
/*
Copyright 2018 The Knative Authors
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Copyright 2018 The Knative Authors
Copyright 2019 The Knative Authors

contrib/gitlab/pkg/reconciler/resources/service.go Outdated
@@ -0,0 +1,72 @@
/*
Copyright 2018 The Knative Authors
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Copyright 2018 The Knative Authors
Copyright 2019 The Knative Authors

contrib/gitlab/pkg/reconciler/webhook_client.go Outdated
@@ -0,0 +1,123 @@
/*
Copyright 2018 The Knative Authors
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Copyright 2018 The Knative Authors
Copyright 2019 The Knative Authors

Copy link
Author

Choose a reason for hiding this comment

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

Wow, thanks

@vincent-pli vincent-pli force-pushed the add-gitlab-source-to-contrib branch from 8c1b5c0 to dcea5c8 Compare May 13, 2019 09:31
@knative-metrics-robot
Copy link

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

File Old Coverage New Coverage Delta
contrib/gitlab/pkg/adapter/adapter.go Do not exist 90.4%
contrib/gitlab/pkg/apis/sources/v1alpha1/gitlab_types.go Do not exist 100.0%
contrib/gitlab/pkg/apis/sources/v1alpha1/register.go Do not exist 100.0%

@vincent-pli vincent-pli mentioned this pull request May 22, 2019
Closed
@grantr
Copy link
Contributor

grantr commented May 22, 2019

/cc @sebgoa

@sebgoa are you interested in reviewing this, since you've written a GitLab source in the past?

@grantr
Copy link
Contributor

grantr commented May 22, 2019

Hi @vincent-pli! I'm sorry this hasn't been approved yet. 😞 We've been busy with Kubecon prep, but once that's done (this week) we should be back to normal.

@sebgoa
Copy link
Contributor

sebgoa commented May 22, 2019

So indeed there is already a gitlabsource at gitlab.com:
https://gitlab.com/triggermesh/gitlabsource

It is listed in the table at:
https://github.com/knative/docs/tree/master/docs/eventing/sources

I thought the plan was not to push all sources to this repo, otherwise I would have moved the source a long time ago.

@grantr
Copy link
Contributor

grantr commented May 22, 2019

@vincent-pli did you know about the GitLab source @sebgoa mentioned before you started work in this PR? I'm curious because if you didn't that sounds like a bug in our docs.

@vincent-pli
Copy link
Author

@sebgoa @grantr
Thanks a lot for the comments.
Actually I test and read the project: https://gitlab.com/triggermesh/gitlabsource, wonderful project @sebgoa 😄 .
The reason why I try to move it is that, with our experience in China, most enterprise user adopter Gitlab rather than Github and they do not want to install overmuch projects.
That's why we need an all-in-one package.

If it's conflict with the strategy, I could persuade our customers, thanks.

@sebgoa
Copy link
Contributor

sebgoa commented May 23, 2019

So I propose we move https://gitlab.com/triggermesh/gitlabsource in this knative repo and we all start to work on the same codebase.

It seems a waste to duplicate efforts.

@grzesiek
Copy link

@vincent-pli Thanks for working on that! 💜

GitLab backend engineer working mostly on GitLab Serverless here.

Let me spend some time reviewing this pull request, it looks really interesting! We would like to understand better how can we combine https://gitlab.com/triggermesh/gitlabsource with this and the work we are doing at GitLab to make most of what Knative offers!

grzesiek
grzesiek reviewed May 27, 2019
View reviewed changes
contrib/gitlab/cmd/receive_adapter/main.go
"os"

gitlab "github.com/knative/eventing-sources/contrib/gitlab/pkg/adapter"
gl "gopkg.in/go-playground/webhooks.v5/gitlab"
Copy link

Choose a reason for hiding this comment

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

Interesting, I wasn't aware that this library exists! I wonder what happens if we plan to actually send webhooks as signed JWTs 🤔

Copy link
Author

Choose a reason for hiding this comment

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

😄

contrib/gitlab/config/300-gitlabsource.yaml
- job_events
- pipeline_events
- wiki_page_events
type: string
Copy link

Choose a reason for hiding this comment

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

I wonder if this actually makes sense to prefix these events with a webhook: prefix, or something like this. We are currently only using webhooks, but I can see us adding more Serverless events. I don't have a strong opinion about doing that right now, something to think about in terms of maintaining backwards compatibility easier.

Copy link
Author

Choose a reason for hiding this comment

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

Make sense if we try to support more event source

contrib/gitlab/pkg/adapter/adapter.go
@@ -0,0 +1,144 @@
/*
Copy link

Choose a reason for hiding this comment

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

Should it be adapter/webhooks.go? 🤔

contrib/gitlab/pkg/adapter/adapter.go
url = pe.ObjectAttributes.URL
case gl.BuildEvents:
be := payload.(gl.BuildEventPayload)
url = be.Repository.URL
Copy link

Choose a reason for hiding this comment

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

That is really confusing that build events do not have object_attributes in the payload, but pipelines events do. This is not consistent across events, perhaps we should open an issue in GitLab about that.

Copy link
Author

Choose a reason for hiding this comment

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

Agree with you

contrib/gitlab/pkg/adapter/adapter.go
log.Printf("url %s", url)
//Hack here, since gitlab/githab could use ssh url. it's not typical URL style, like this: git@github.com:knative/eventing-sources.git
//I guess it could be setting in the gitlab/github, but leave the code here to prevent exception
if url != "" {
Copy link

Choose a reason for hiding this comment

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

Can you explain in which cases this problem can occur? We do have git_http_url in a few objects, but I see that we are not using it here 🤔

Copy link
Author

Choose a reason for hiding this comment

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

If use ssh url like "git@github.com:tektoncd/pipeline.git", it's not a typical url which could parse by net/url , so I hack it 😄

contrib/gitlab/pkg/client/clientset/versioned/fake/clientset_generated.go

// Code generated by client-gen. DO NOT EDIT.

package fake
Copy link

Choose a reason for hiding this comment

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

I'm just curious, since this is generated code, has it been generated by go-client directly, or have you used operator-sdk to generate it?

Copy link
Author

Choose a reason for hiding this comment

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

It's generated by go-client directly

@grzesiek
Copy link

@sebgoa What is your take on moving forward with this? Code here looks nice, it seems to be a little better unit tested and maybe is a little more complete than https://gitlab.com/triggermesh/gitlabsource (eg. see reconciler, fake client etc). Is there anything we could add here to make it better / more complete? Are we missing something that is available in https://gitlab.com/triggermesh/gitlabsource but is not available here? 🤔

grzesiek
grzesiek reviewed May 27, 2019
View reviewed changes
contrib/gitlab/pkg/reconciler/gitlabsource.go
secretToken: args.secretToken,
}

projectName, err := getProjectName(args.source.Spec.ProjectURL)
Copy link

Choose a reason for hiding this comment

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

What happens if someone actually wants to add a group-level cluster and use group-level webhooks? Do we plan to support that?

@vincent-pli
Copy link
Author

@grzesiek
Thanks you for comments, they are valuable.
As I mentioned before, the motivation of this PR is for an all-in-one package.
So we just satisfied our cases. many valuable cases, like support more events, JWT s , group-level are not in our original scope.

We plan add them case by case later 😅

@grzesiek
Copy link

@vincent-pli Our goal here is to make it possible to use this from the GitLab itself, please see https://gitlab.com/gitlab-org/gitlab-ce/issues/62522. I'm really curious what you think about this idea. @sebgoa I'm curious about your opinion too.

@sebgoa sebgoa mentioned this pull request May 31, 2019
Closed
@vincent-pli vincent-pli mentioned this pull request Jun 11, 2019
Closed
@matzew
Copy link
Member

matzew commented Oct 11, 2019

what's the state here, compared to #450 ?

@n3wscott
Copy link
Contributor

Should we close this in favor of https://gitlab.com/triggermesh/gitlabsource ?

@matzew
Copy link
Member

matzew commented Jan 15, 2020

Should we close this in favor of https://gitlab.com/triggermesh/gitlabsource ?

@n3wscott there was no reaction, in over a month ... closing it ?

@vincent-pli
Copy link
Author

Closed, thanks

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@srvaroa srvaroa srvaroa left review comments

@grzesiek grzesiek grzesiek left review comments

@mattmoor-sockpuppet mattmoor-sockpuppet mattmoor-sockpuppet left review comments

@evankanderson evankanderson Awaiting requested review from evankanderson

@n3wscott n3wscott Awaiting requested review from n3wscott

@sebgoa sebgoa Awaiting requested review from sebgoa

Assignees

@evankanderson evankanderson

@n3wscott n3wscott

Labels
cla: yes Indicates the PR's author has signed the CLA. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. 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.