Skip to content
This repository has been archived by the owner on Mar 8, 2020. It is now read-only.

[WIP] drivers jenkins -> change trigger to github webhook #444

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

lwsanty
Copy link
Member

@lwsanty lwsanty commented Oct 30, 2019

Note this issue must be closed first https://github.com/src-d/infrastructure/issues/1249

Signed-off-by: lwsanty [email protected]


This change is Reviewable

@lwsanty lwsanty requested a review from a team as a code owner October 30, 2019 13:16
@lwsanty lwsanty self-assigned this Oct 30, 2019
Copy link
Member

@dennwc dennwc left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 3 files at r1.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @lwsanty)


README.md, line 183 at r1 (raw file):

## License

GPLv3, see [LICENSE](LICENSE)

It's unrelated, so please revert.


etc/skeleton/Jenkinsfile.tpl, line 46 at r1 (raw file):

        [key: 'ref', value: '$.ref']
      ],
      token: '{{.Manifest.Language}}-driver',

Wondering, what this "token" does?


etc/skeleton/Jenkinsfile.tpl, line 53 at r1 (raw file):

      regexpFilterText: '$ref',
      regexpFilterExpression: 'refs/heads/' + BRANCH_NAME

Will it trigger on tags?

Copy link
Member Author

@lwsanty lwsanty left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @dennwc)


README.md, line 183 at r1 (raw file):

Previously, dennwc (Denys Smirnov) wrote…

It's unrelated, so please revert.

Done.


etc/skeleton/Jenkinsfile.tpl, line 46 at r1 (raw file):

There is a special token parameter. When supplied, the invocation will only trigger jobs with that exact token. The token also allows invocations without any other authentication credentials.

https://wiki.jenkins.io/display/JENKINS/Generic+Webhook+Trigger+Plugin


etc/skeleton/Jenkinsfile.tpl, line 53 at r1 (raw file):

Previously, dennwc (Denys Smirnov) wrote…

Will it trigger on tags?

according to https://github.com/src-d/infrastructure/issues/1249#issuecomment-547509076
our middle-ware will work only with push events, so the answer is no

@lwsanty lwsanty requested a review from dennwc October 30, 2019 13:55
Copy link
Member

@dennwc dennwc left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r2.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @lwsanty)


etc/skeleton/Jenkinsfile.tpl, line 46 at r1 (raw file):

Previously, lwsanty wrote…

There is a special token parameter. When supplied, the invocation will only trigger jobs with that exact token. The token also allows invocations without any other authentication credentials.

https://wiki.jenkins.io/display/JENKINS/Generic+Webhook+Trigger+Plugin

I see. So now I'm also concerned with https://github.com/src-d/infrastructure/issues/1249#issuecomment-547819998.


etc/skeleton/Jenkinsfile.tpl, line 53 at r1 (raw file):

Previously, lwsanty wrote…

according to https://github.com/src-d/infrastructure/issues/1249#issuecomment-547509076
our middle-ware will work only with push events, so the answer is no

But we may need triggers on tags as well. It should be possible to pass all events from the Github.

Copy link
Member Author

@lwsanty lwsanty left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dennwc)


etc/skeleton/Jenkinsfile.tpl, line 46 at r1 (raw file):

Previously, dennwc (Denys Smirnov) wrote…

I see. So now I'm also concerned with https://github.com/src-d/infrastructure/issues/1249#issuecomment-547819998.

You should not, middleware will be protected by the secret


etc/skeleton/Jenkinsfile.tpl, line 53 at r1 (raw file):
hm, I double checked

Branch pushes and repository tag pushes also trigger webhook push events
https://developer.github.com/webhooks/

So tags will be included.

@lwsanty lwsanty requested a review from dennwc October 30, 2019 15:06
Copy link
Member

@dennwc dennwc left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @lwsanty)


etc/skeleton/Jenkinsfile.tpl, line 46 at r1 (raw file):

Previously, lwsanty wrote…

You should not, middleware will be protected by the secret

You mean that the token will be send inside the cluster, while a real secret will be used to expose that middleware?


etc/skeleton/Jenkinsfile.tpl, line 53 at r1 (raw file):

Previously, lwsanty wrote…

hm, I double checked

Branch pushes and repository tag pushes also trigger webhook push events
https://developer.github.com/webhooks/

So tags will be included.

Awesome, thanks!

Copy link
Member Author

@lwsanty lwsanty left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dennwc)


etc/skeleton/Jenkinsfile.tpl, line 46 at r1 (raw file):

Previously, dennwc (Denys Smirnov) wrote…

You mean that the token will be send inside the cluster, while a real secret will be used to expose that middleware?

Generally yes, I just don't know what means real :)

@lwsanty lwsanty requested a review from dennwc October 31, 2019 08:51
Copy link
Member

@dennwc dennwc left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved


etc/skeleton/Jenkinsfile.tpl, line 46 at r1 (raw file):

Previously, lwsanty wrote…

Generally yes, I just don't know what means real :)

"Real" means the one that provides security ("go-driver" as a token doesn't).

So OK, assuming we only send insecure tokens inside our own cluster, LGTM.

Copy link
Member Author

@lwsanty lwsanty left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion

a discussion (no related file):
blocked until https://github.com/src-d/infrastructure/issues/1249


Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants