Add PR branch validation

[ivanayov]

This checks if the branch a pull request is raised from is non-master. Validates, as well, if a pull request is raised against the default branch. If the validation is not successful, the pull request labels are applied: "review/target-branch" if the target branch is not the default and "review/source-branch" if the source branch is named "master". The source branch validation leaves a warning comment, suggesting to raise a new pull request from a named branch

Signed-off-by: Ivana Yovcheva (VMware) iyovcheva@vmware.com

Raised in issue #17

[alexellis]

Check how to update pull status with go-github

This scenario cannot be recovered without raising a new PR from a different remote branch so I'd suggest not using the status API and instead using a label.

Via @rgee0 in the issue description:

Upon submission of a PR Derek should check the originating branch name, and if he finds master the PR closed with a message advising the user that they should re-submit from a non-master branch. Also, check that pull request is against master / default.

[alexellis]

@iyovcheva looks like the PR is breaking on CI. We should run a local docker build before pushing commits up to GitHub - it can reveal build/test errors ahead of time.

[ivanayov]

@alexellis @rgee0 can you please review?

[alexellis]

We don't have any assign reviewer capability. Feel free to raise an issue suggesting it

[alexellis]

Let's make the minor changes suggested today and then do some e2e testing:

• Create your own GitHub App - and save the cert / pem
• Install it on one of your repos
• Run ngrok and point the derek function back to your GitHub app or run your OpenFaaS somewhere accessible like DigitalOcean

Then trigger the function and check the behaviour.

This will also be an opportunity for us to update the documentation on how to test / build / develop with Derek.

[alexellis]

@iyovcheva I see you pushed some changes. Please could you respond to the comment?

[alexellis]

@iyovcheva please could you run through some end-to-end testing using the instructions above? I pinged 20 days ago and think this would be a useful feature for the project.

While you are deploying Derek for testing it may make sense to take some cropped screenshots and update the testing instructions for future contributors such as @neolit123

cc @rgee0

[alexellis]

@ivanayov end-to-end testing needed on this. I'll mark WIP until we have that done.

Thanks, Alex

[neolit123]

@alexellis @ivanayov

hi, Alex. i spoke briefly with Ivana about this and i could be missing some detail.

would a complete e2e for a bot that handles PRs mean that a certain repository has to be clobbered - i.e. the testing code would create a branch in a repository somewhere, make a change to a file and then submit a PR for this file. the testing code would then validate if the source and destination branches match a criteria. at least that's how a see a complete e2e test.

would it make sense to attempt to handle that via unit tests only. if even possible? thanks.

[alexellis]

End to end testing means that your code is validated running in a live environment showing it does what it says it should. We don't want to discover bugs after releasing a new version of the code and deploying it to "production".

Unit tests are also an important part of regression testing, but it's difficult to produce the correct conditions to show all the moving parts are working as expected.

So for this change deploy Derek to your OpenFaaS instance, point your GitHub App at the endpoint and then add the app to your test repo. After that fire an event to trigger the behaviour.

This should be quick to test.

[neolit123]

ah, so this is a one-shot manual e2e test to verify that this PR works. for some reason i though that the request is to add the branch verification as part of CI.

[rgee0]

I tend to pull up a clean OF instance on Digital Ocean and deploy Derek to it. I put some Ansible together to provision a droplet and deploy OF - https://github.com/rgee0/openfaas-DO

I do have a test repo that does get clobbered, but thats what its there for.

Test the feature partitions: master to master, branch to master, branch to non-master, master to branch. Because this is relatively quick testing I'd also ensure the existing features still function.

The github facility to view and re-send requests also comes in useful.

[neolit123]

just an idea to throw out there; probably considered already.

how about you guys integrate Derek as a continuous deployment?

proposal:

• contributor submits PR to this main Derek repo.
• Travis builds Derek and deploys it somewhere in the cloud.
• this test build in the cloud is statically bound to the following repo to be manually clobbered by users: https://github.com/alexellis/derek-test, or maybe https://github.com/openfaas/derek-test
• the contributor is instructed to test his change in that repo via CI output or a bot message.
• the maintainer can verify the same.

this is great for new contributors who don't have (e.g.) digital ocean accounts or enough knowledge to do the deployment them self. also speeds up the verification process of new features.

potential problem here would be that if multiple PRs are submitted on top of each other the derek-test would operate on the latest Derek that ended up being deployed. the addition of the Derek version command to dump a build SHA is mandatory here.

@rgee0 @alexellis @ivanayov WDYT? let me know if this is any good and if you want me to log an issue for it.

[ivanayov]

@neolit123 This sounds like a good idea.

@mrtind Just tested his PR and has a test environment, so I asked him if he can help with the e2e testing of this as well.

[ivanayov]

@mrtind shared his test environment with me, so I can do e2e test the next days

[sesam]

This PR is for the last item in the feature wish-list from the end of the README, right? Do you think this implementation is complete?

[alexellis]

Hi Simon,

Why do you ask?

I would say we should close the PR as the author is no longer active, and the PR is quite old now and out of sync. I would have to take a detailed look to understand if it's complete.

Alex

[sesam]

Thank you. And I asked at the same time as I started to try making a branch that does the same, but taking into account renaming of files. Also, I saw an opportunity to make the change simpler. From the discussion so far, I'm guessing that this project is more or less active, and I'll continue my path to contribute here while figuring out what github bots can do.