Join GitHub today
GitHub is home to over 31 million developers working together to host and review code, manage projects, and build software together.Sign up
Improved infrastructure for notebook testing #429
I think it is safe to say that none of us are too fond of the
In fact, I've found it a real hinderance personally:
However, there are a few advantages using GitHub:
Here is the overall outline of how it will work if this PR goes to plan:
This would make things a lot easier but the submodule reference in the current system would remain a major problem. Although we could automatically update the reference on our repository we can't help people submitting PRs to us. We don't have the credentials to push to other people's repositories. Other people would still be left with some of the nastiest steps to carry out themselves.
The proposed system is based on these ideas:
How will this work concretely?
1.When running notebook tests, Travis uses the
I bet it sounds complicated but if it works, it will make developing new notebooks much easier.
referenced this pull request
Jan 29, 2016
It's been a pretty huge effort (all happening behind the scenes) to get this working. I'll be happy to see the end of it!
Naturally, we quickly realized the proposal above wasn't going to work. The main issue was the suggestion is it didn't tackle merges. How would you make the PR branch on holoviews-data become the main one when you click 'merge'?
So we now have a new variation on this approach.
The idea is that build step handles merging. The way it does this, is it has to do quite a lot behind the scenes:
Note that this means we should no longer use the 'Merge' button on github!
This is probably not an issue as none of us are so irresponsible enough to merge a PR without the tests going green, right? :-)
To get the tests green, we need to use buildbot anyway so enabling the merge step there helps avoid error. If you are uncertain and don't trust buildbot and want to see the tests go green for yourself, you can leave the 'Merge' checkbox unticked. Then if all is well and you decide to merge, this needs to be done via buildbot.
Anyway, I hope that makes sense and I think I am 95% the way to the final system. I need to handle #PR0 (which represents master) and some of the last merge steps need to be implemented. Also, do you think we should make the default PR number -1 so you have to enter 0 for master? I worry a little about people accidentally entering the wrong PR number...
Note: Buildbot checks all the jobs for the build that is merging into master goes green. i.e if the PR were merged to master, would the tests pass? As you can see with this PR here, that doesn't mean the other set of jobs (branch test) will pass but that will almost be due to transient errors if the other set of builds is passing, right?
I'm fairly certain that is not possible in any sane or legal way. :-p
What might be possible is this:
I could set up a GitHub webhook to trigger the necessary merge of
The script could then listen for events from github and when it receives one, trigger a build on buildbot as described in this link (essentially using
Buildbot would then run a separate builder to:
1 Find the travis build that is triggered on master by the merge event.
I see some clear advantages:
I will consider this possibility as I have the tools for it ready and I was hadn't really finished or tested the merging steps anyway.
The proposed builder would also mean I could keep the current one to handle PRs only (no need for 0 to mean 'master'). The new builder would be dedicated to updating the reference data for master. E.g if you see a travis build that fails on master, check travis.holoviews.org and decide the changes are ok, you could then just force this builder to run to make the update.
I've just tested all the basic ideas. I've made myself a repo and every time an event happens there on GithHib, I can use that to trigger a build.
This opens up some really interesting/crazy possibilities. I could easily set things up so that the only thing needed to update the data for a PR is to type a special string such as:
This could then update the test data and restart the last Travis build. This is either very convenient or a bit nuts depending on your perspective! It would also sidestep the issue of given out user credentials entirely...
I think this could be really great if we take a few precautions:
Anything else? With this suggestion, it really couldn't get more automated...
This idea could extend to website building given the following conditions;
With these conditions in place, I can imagine that all you need to do to update the website for a PR is type:
Any thoughts on this idea?
Edit: Having a BUILDBOT: SLAVE-STOP would be a good idea (wouldn't offer a SLAVE-START though!)
The merge wouldn't need a special string: that I can detect and handle without any intervention at all. The special strings could be anywhere in the comment text of a PR and processed when the comment is posted. The options would be to update the test data for the current PR or rebuild the website for the current PR. As you say, I think it would be very convenient but I would like to hear what Philipp thinks of the idea...
All sounds great to me. What we've got now is already miles better than what we had before. The only other thing I could wish for is a status hook for PR builds, which indicates whether a particular build is in the cache and can therefore be merged, just like the Travis and Coveralls checks below. Then you'd only get a green go-ahead to merge when the tests for that PR are either passing or you've recently built them and the new data is cached.
Hmmm. I don't think it would be hard to periodically check if the latest PR build is in the cache (already have code that does this) and serve one particular icon if it is available and another if it isn't. Then at the start of a PR, we could add a bit of markdown to show that icon. Would that do the trick?
According to this you can POST up to 1000 statuses for each commit so we could add a status when something is added to the cache and send a fail event when a build is deleted. Not a priority though.
Edit: We can add another webhook Lambda to signal whenever a build is added to preview.holoviews.org, which a) sends a 'pass' event to the tested commit and b) sends a fail event to the commit corresponding to the build that was just deleted. Github should then update the PRs appropriately.
I think I'm happy with the travis changes in this PR. It seems to work and does what we need.
I can't get the tests passing here due to changes elsewhere (some data used in one of the tutorials has moved). I think we can merge and start testing this system on a real PR!
pushed a commit
this pull request
Feb 2, 2016
Feb 2, 2016
It is all working well so I've pushed the files to the buildbot branch of ioam-builder. If could always do with more polishing but I am very pleased that all the important functionality now works.
Note that we have to be very careful not to accidentally push our secret tokens! I've censored them out so you should spot them in any diffs before you push.
I've decided to use this issue to log changes to the infrastructure. In the past few days, a number of things needed to be fixed:
All these changes have been pushed to the buildbot branch of ioam-builder.