thefederationinfo/github-integration

Fork 0

Implement opt-in/out for pull requests #8

Merged

Zauberstuhl merged 5 commits from opt_in-out into master

2018-03-18 18:14:56 +01:00

Zauberstuhl commented

2018-03-17 12:52:05 +01:00

(Migrated from github.com)

closes #4

That will check the PR title, body-message and label names. It would require a bit more magic if
we want to check commit messages as well but Is that actually necessary? @SuperTux88

closes #4 ![untitled](https://user-images.githubusercontent.com/652428/37555139-09da31b2-29e3-11e8-98c4-c5a4d68d6194.png) That will check the PR title, body-message and label names. It would require a bit more magic if we want to check commit messages as well but Is that actually necessary? @SuperTux88

jaywink (Migrated from github.com) approved these changes

2018-03-17 14:33:42 +01:00

jaywink (Migrated from github.com) left a comment

Looks good to me! 🎉

Looks good to me! :tada:

SuperTux88 commented

2018-03-17 15:30:22 +01:00

(Migrated from github.com)

First: thanks for the fast implementation @Zauberstuhl (probably faster than I can integrate that in our projects ;) )

Lets discuss opt-in and opt-out separately (I think they should work a bit differently because they have different use-cases):

Opt-in

I think [ci] or a ci label is too generic. Since I would use this opt-in for the diaspora main-repo and there all PRs have CI, but only a small percentage of them need federation-CI. So it would be weird if it is needed to add a ci label to activate the extra federation tests (but this label doesn't have any influence on other CI).

The best solution for me would be when we could configure the label needed for that (as you proposed in https://github.com/thefederationinfo/github-integration/issues/4#issuecomment-373563985). We already have a federation label for issues that need federation changes and it would make a lot of sense to me to just add this label to PRs with federation changes too.

If you don't want to make the opt-in label configurable, at least name it something including "federation", so it's clear that this is only for federation tests.

Also: Checking PR title and message and labels is OK to me here (I probably would use federation-label if I can). Checking commit-messages could be complicated, especially since I think a PR should stay opted-in once it contains federation related changes.

Opt-out

Here I think the generic [ci skip] is perfectly fine and enough. I would use this opt-out for the diaspora_federation repo and when I do changes that don't need CI (for example documentation changes), I want to disable all CI (travis + federation CI), so it's the easiest to just use the same keyword as travis, so I can disable both at once.

It would require a bit more magic if we want to check commit messages as well but Is that actually necessary?

I think travis only checks the commit-message (for a push to a branch directly or for a PR the last commit counts). When adding commits to a PR, it checks the new commits and if it doesn't contain a [ci skip] anymore it runs the tests. I didn't test this much and didn't try if PR title or message work too, but it always worked with the commit-message.

For me it's probably fine when you only check the PR title/message, but I think you receive all commit-messages in the hook too (I never built something with github hooks, so I'm not 100% sure), so I think it should be relatively easy to just check if the last commit-message for this hook-call contains a [ci skip] (only the last commit counts if there are multiple commits). If you parse the PR title/message and it contains a [ci skip] it should probably have higher priority than the commits (so when it contains [ci skip] in the title, never run, and ignore the commits).

And if you want to be super-cool you also add both [ci skip] and [skip ci] as it works for travis (see https://github.com/travis-ci/travis-ci/issues/911), but that would be only nice to have and not needed ;)

First: thanks for the fast implementation @Zauberstuhl (probably faster than I can integrate that in our projects ;) ) Lets discuss opt-in and opt-out separately (I think they should work a bit differently because they have different use-cases): ### Opt-in I think `[ci]` or a `ci` label is too generic. Since I would use this opt-in for the `diaspora` main-repo and there all PRs have CI, but only a small percentage of them need federation-CI. So it would be weird if it is needed to add a `ci` label to activate the extra federation tests (but this label doesn't have any influence on other CI). The best solution for me would be when we could configure the label needed for that (as you proposed in https://github.com/thefederationinfo/github-integration/issues/4#issuecomment-373563985). We already have a `federation` label for issues that need federation changes and it would make a lot of sense to me to just add this label to PRs with federation changes too. If you don't want to make the opt-in label configurable, at least name it something including "federation", so it's clear that this is only for federation tests. Also: Checking PR title and message and labels is OK to me here (I probably would use `federation`-label if I can). Checking commit-messages could be complicated, especially since I think a PR should stay opted-in once it contains federation related changes. ### Opt-out Here I think the generic `[ci skip]` is perfectly fine and enough. I would use this opt-out for the `diaspora_federation` repo and when I do changes that don't need CI (for example documentation changes), I want to disable all CI (travis + federation CI), so it's the easiest to just use the same keyword as travis, so I can disable both at once. > It would require a bit more magic if we want to check commit messages as well but Is that actually necessary? I think travis only [checks the commit-message](https://docs.travis-ci.com/user/customizing-the-build/#Skipping-a-build) (for a push to a branch directly or for a PR the last commit counts). When adding commits to a PR, it checks the new commits and if it doesn't contain a `[ci skip]` anymore it runs the tests. I didn't test this much and didn't try if PR title or message work too, but it always worked with the commit-message. For me it's probably fine when you only check the PR title/message, but I think you receive all commit-messages in the hook too (I never built something with github hooks, so I'm not 100% sure), so I think it should be relatively easy to just check if the last commit-message for this hook-call contains a `[ci skip]` (only the last commit counts if there are multiple commits). If you parse the PR title/message and it contains a `[ci skip]` it should probably have higher priority than the commits (so when it contains `[ci skip]` in the title, never run, and ignore the commits). And if you want to be super-cool you also add both `[ci skip]` and `[skip ci]` as it works for travis (see https://github.com/travis-ci/travis-ci/issues/911), but that would be only nice to have and not needed ;)

Zauberstuhl commented

2018-03-17 18:33:22 +01:00

(Migrated from github.com)

@SuperTux88 thanks for your input! I changed following now:

make trigger flags like ci and ci skip customizable
check commit messages as well

I was too lazy making another API call to github. That was the reason why I asked whether we need it or not. In retrospect it was stupid and being able to search commit messages could come in handy.

I tested manually the current implementation #9 and if there are no further objections/suggestions I would merge!

And if you want to be super-cool you also add both [ci skip] and [skip ci] as it works for travis (see travis-ci/travis-ci#911), but that would be only nice to have and not needed ;)

Haha I saw that too. I guess with custom flags it should be obsolete now.

First: thanks for the fast implementation @Zauberstuhl (probably faster than I can integrate that in our projects ;) )

Thats totally fine ;)

Something not related to this PR; @SuperTux88 do you want to join thefederationinfo organization?
I think it would make sense since you are basically the diaspora-federation-master-of-disaster guy
and I could harass you more efficiently with PR review requests ;)

@SuperTux88 thanks for your input! I changed following now: - make trigger flags like `ci` and `ci skip` customizable - check commit messages as well I was too lazy making another API call to github. That was the reason why I asked whether we need it or not. In retrospect it was stupid and being able to search commit messages could come in handy. I tested manually the current implementation #9 and if there are no further objections/suggestions I would merge! ![untitled](https://user-images.githubusercontent.com/652428/37558218-dcbcc7ca-2a10-11e8-9bdf-ec12c75f4caa.png) > And if you want to be super-cool you also add both [ci skip] and [skip ci] as it works for travis (see travis-ci/travis-ci#911), but that would be only nice to have and not needed ;) Haha I saw that too. I guess with custom flags it should be obsolete now. > First: thanks for the fast implementation @Zauberstuhl (probably faster than I can integrate that in our projects ;) ) Thats totally fine ;) --- Something not related to this PR; @SuperTux88 do you want to join thefederationinfo organization? I think it would make sense since you are basically the diaspora-federation-master-of-disaster guy and I could harass you more efficiently with PR review requests ;)

SuperTux88 commented

2018-03-17 19:06:11 +01:00

(Migrated from github.com)

Looks good to me 👍

I was too lazy making another API call to github.

I didn't know you need API calls for that, but as said: I have no idea how these integrations work in detail. But cool that it now works :)

I guess with custom flags it should be obsolete now.

I think the problem is, when different people want to use different flags (because they are used to the one or the other from using with travis). ;) Custom flags would solve this when they can be a regex.

But it's perfectly fine as it is now and I think it fits all my needs. It could always be improved in the future in case somebody really needs it.

do you want to join thefederationinfo organization?
I think it would make sense since you are basically the diaspora-federation-master-of-disaster guy
and I could harass you more efficiently with PR review requests ;)

I don't know what your plan is with this organization and I don't know how much time I'll have or if I could help much (probably not much with this repo, since I don't have much go experience, but maybe with the diaspora integration in federation-tests). But if you think it would help feel free to add me. :)

Looks good to me :+1: > I was too lazy making another API call to github. I didn't know you need API calls for that, but as said: I have no idea how these integrations work in detail. But cool that it now works :) > I guess with custom flags it should be obsolete now. I think the problem is, when different people want to use different flags (because they are used to the one or the other from using with travis). ;) Custom flags would solve this when they can be a regex. But it's perfectly fine as it is now and I think it fits all my needs. It could always be improved in the future in case somebody really needs it. > do you want to join thefederationinfo organization? > I think it would make sense since you are basically the diaspora-federation-master-of-disaster guy and I could harass you more efficiently with PR review requests ;) I don't know what your plan is with this organization and I don't know how much time I'll have or if I could help much (probably not much with this repo, since I don't have much go experience, but maybe with the diaspora integration in [federation-tests](https://github.com/thefederationinfo/federation-tests)). But if you think it would help feel free to add me. :)

SuperTux88 (Migrated from github.com) approved these changes

2018-03-17 19:07:44 +01:00

SuperTux88 (Migrated from github.com) left a comment

I can't judge the code and I didn't test it, but the screenshot looks like it fits my needs 👍

I can't judge the code and I didn't test it, but the screenshot looks like it fits my needs :+1:

Zauberstuhl commented

2018-03-18 04:47:59 +01:00

(Migrated from github.com)

I didn't know you need API calls for that, but as said: I have no idea how these integrations work in detail. But cool that it now works :)

The workflow would be as following:

You (the owner) of a repository can grant access to the repository we want to cover.
The integration server requests admin:repo_hook and repo:status permissions (see https://developer.github.com/apps/building-oauth-apps/scopes-for-oauth-apps/ and https://github.com/thefederationinfo/github-integration/blob/master/server.go#L41)

admin:repo_hook

This is required cause we want to automatically create a repository webhook which reports created pull-requests. The webhook can only handle pull request events see https://github.com/thefederationinfo/github-integration/blob/master/frontend.go#L117

repo:status

We need this to update status messages on commits. A commit status message is what you see in a pull request e.g. has travis passed all tests.

See https://developer.github.com/v3/repos/statuses/#create-a-status

To summarize it; The server will now be able to receive PR requests from your repo and can update commit statuses. If a pull request is made, github will report to the-federation.info and the server can then decide whether we should trigger a travis build with tests defined in here: https://github.com/thefederationinfo/federation-tests

Custom flags would solve this when they can be a regex.

mh thats true lets solve this in the future :) #10

I don't know how much time I'll have or if I could help much

Don't worry I am glad if you simply report your thoughts if I am trying to improve federation again ;)

Time for bed now 💤

> I didn't know you need API calls for that, but as said: I have no idea how these integrations work in detail. But cool that it now works :) The workflow would be as following: You (the owner) of a repository can grant access to the repository we want to cover. The integration server requests `admin:repo_hook` and `repo:status` permissions (see https://developer.github.com/apps/building-oauth-apps/scopes-for-oauth-apps/ and https://github.com/thefederationinfo/github-integration/blob/master/server.go#L41) ### admin:repo_hook This is required cause we want to automatically create a repository webhook which reports created pull-requests. The webhook can only handle pull request events see https://github.com/thefederationinfo/github-integration/blob/master/frontend.go#L117 ### repo:status We need this to update status messages on commits. A commit status message is what you see in a pull request e.g. has travis passed all tests. See https://developer.github.com/v3/repos/statuses/#create-a-status --- To summarize it; The server will now be able to receive PR requests from your repo and can update commit statuses. If a pull request is made, github will report to the-federation.info and the server can then decide whether we should trigger a travis build with tests defined in here: https://github.com/thefederationinfo/federation-tests --- > Custom flags would solve this when they can be a regex. mh thats true lets solve this in the future :) #10 > I don't know how much time I'll have or if I could help much Don't worry I am glad if you simply report your thoughts if I am trying to improve federation again ;) Time for bed now :zzz:

👍 1