Submission bot wishlist

[adam3smith]

CC @rmzelle @inukshuk

Follow up of citation-style-language/styles#1088

1. Allow customized, automated message on travis return
2. Include instructions on how to update pull request specific for each pull request (i.e. with link to correct branch in fork)
3. Allow customization of message depending on Travis errors: I'm thinking we'll create human-written instructions for the most common travis errors
4. Have a separate warning category if possible, where we could point to common issues which don't cause a travis fail, such as missing documentation or default locale.

Here's how I'm roughly envisioning a sample message:

Thank you for submitting to the Citation Styles Language repository. Our automated test suite has found some errors in your submission.
Please make sure that your filename, id, and link follows the conventions outlined in 1,2,4,5,6 here.

While not technically required, we also ask that you include a link to online documentation for the style with rel="documentation" as per 10. here.

You can update your pull request by editing your style [here --include link to correct branch]. Click the pencil icon at the top right of that page.

Please let us know when you're done with all edits, as we don't get notified for updated pull requests. Feel free to post here if you need any help.

[rmzelle]

Good start. Some additional thoughts:

• maybe we can reuse the https://github.com/csl-bot account to post the reports?
• it would be good if there was a special warning if the Travis CI build failed because of other reasons. Sometimes a build just doesn't complete, and it would be nice to let the submitter know it's not their fault. E.g:

Thank you for your submission. Unfortunately, our automated Travis CI test suite failed for unknown reasons. Please wait until the CSL team restarts the build.

• pull requests can cover multiple files, so it would be good to give a per-file list of errors. It would be good to have cut-off based on e.g. the number of files, or the number of lines in the report, in case somebody really screws up a commit. E.g. (based on https://travis-ci.org/citation-style-language/styles/builds/55675997):

Thank you for your submission. We review each submission by hand, but our automated Travis CI test suite already found some issues:

• padagogische-hochschule-vorarlberg
  • is not a valid CSL style. Please see https://github.com/citation-style-language/styles/wiki/Validation and make sure your style validates
  • calls an undefined macro: "year-date"

You can update this pull request by visiting the style link(s) above and clicking the pencil icon.

• when new commits trigger additional bot reports in a given pull request, it would be nice if these subsequent reports are more concise. E.g. text like "Thank you for your submission" doesn't need to be repeated over and over again.
• The "Please let us know when you're done with all edits" should not be necessary if we have the bot, since we could have it always post, on fail and success.

[rmzelle]

Also, I imagine that there is a wider interest in a tool like this that can parse Travis CI/RSpec reports and post back to GitHub, so if you can make the bot more modular/configurable without too much extra effort, that might be good to keep in mind as well.

[rmzelle]

I would also be nice to provide a link to the validator in case of invalid styles. E.g. for https://github.com/peboeck/styles/blob/patch-8/padagogische-hochschule-vorarlberg.csl, provide the link http://validator.citationstyles.org/?url=https%3A%2F%2Fraw.githubusercontent.com%2Fpeboeck%2Fstyles%2Fpatch-8%2Fpadagogische-hochschule-vorarlberg.csl&version=1.0.1

[adam3smith]

Yes, all good additions/changes, no objections. Very much agree on trying to make this easily adaptable for other people.

[inukshuk]

Do you have any preference regarding the implementation language? I'm leaning towards Ruby just in case it will be necessary to run any of the tests locally.

[rmzelle]

Ruby probably makes the most sense.

[inukshuk]

Do you think it would make sense to use two hooks: one for GitHub Pull Requests and one for Travis CI builds. This way, we could respond to each Pull Request with a general comment along the lines of the example @adam3smith posted above; whilst the response to each Travis build could focus solely on the results of the build (without the need to distinguish between the initial Pull Request and later commits). Do you think that would make sense?

[rmzelle]

Yeah, sure. Modular is good :).

[rmzelle]

It would also provide immediate feedback upon creation of a pull request, which is a good thing.

[inukshuk]

@rmzelle have you decided if you'd like to reuse @csl-bot to post Sheldon's comments? (I'm all for it) Can you create an access token for me to use to post comments as @csl-bot for testing? (We just need a secure channel for you to send it to me... perhaps a GPG encrypted mail?)

[rmzelle]

@dstillman, can you help @inukshuk out with access to the @csl-bot account?

[rmzelle]

We could also create a separate account, like "sheldon-bot" or "Shelbot" (the latter seems to be the official name for Sheldon's robot: http://bigbangtheory.wikia.com/wiki/The_Cruciferous_Vegetable_Amplification ; both usernames are available on GitHub), which would allow us to give the bot a more human face.

Would two accounts be preferable, security-wise?

[inukshuk]

Humble beginnings: inukshuk/styles#3

Will hopefully have a first version for you to experiment with in a couple of days!

[rmzelle]

Will hopefully have a first version for you to experiment with in a couple of days!

Is there anything I should wait for, or can I start submitting PRs for your repo already (e.g. to change the text)?

[inukshuk]

Let's wait until I have the build hooks ready -- I'll be much quicker to add/adjust features then.

[rmzelle]

Okay. Already looks very promising!

[rmzelle]

I added headers to the various submission criteria at https://github.com/citation-style-language/styles/wiki/Style-Requirements, so they now have HTML anchors we can link to. E.g. https://github.com/citation-style-language/styles/wiki/Style-Requirements#1---title-abbreviations

[adam3smith]

yes, thank @inukshuk very excited about what I'm seeing so far.

[inukshuk]

I've added build passed/failed templates now, so I think we can start thinking about the actual content of the templates and how to roll this out. Currently the bot is hosted on Heroku and is automatically updated if you push any changes and the tests pass on Travis CI. I think this is a pretty good setup, because this will allow us to make changes to a template and just do a git push to deploy it.

Since we want to enable messages on failed builds I will make a PR to the distribution-updater bot to ignore failed builds, OK?

The payload sent by Travis does not give us any details on why a build failed; in the long run we could consider having our bot checkout the commit and do its own testing, but it probably makes more sense to simply improve the output of the test suite.

[inukshuk]

Here is an example with failing and passing builds: inukshuk/styles#4

[rmzelle]

Since we want to enable messages on failed builds I will make a PR to the distribution-updater bot to ignore failed builds, OK?

I'm confused. csl-bot already only pushes to the "styles-distribution" if the Travis build of the "styles" repo succeeds. What needs to be changed?

The payload sent by Travis does not give us any details on why a build failed

It would be nice if we could steer users to the relevant sections of https://github.com/citation-style-language/styles/wiki/Style-Requirements, though. I found travis-ci/travis-ci#239, which shows that structured test metadata is unfortunately not to be expected anytime soon. The travis.rb library allows for easy access to the build log (https://github.com/travis-ci/travis.rb#logs), although I don't know if that's any easier than just checking the log online. We can probably use some regex to identify which tests failed and format the GitHub feedback comment appropriately, no?

[inukshuk]

I'm confused. csl-bot already only pushes to the "styles-distribution" if the Travis build of the "styles" repo succeeds. What needs to be changed?

Well, that's it: if we want to receive notifications on failed builds in the future, we will need to change the Travis configuration. Once we do that, the styles-distribution hook will be called for failed builds as well, so we should make sure those will be ignored before we change the configuration.

[rmzelle]

Isn't it easier to add a second hook that gets called for all builds?

[inukshuk]

I don't know if it is possible to have different settings per hook; at least that is my interpretation of the docs at: http://docs.travis-ci.com/user/notifications/#Webhook-notification

[inukshuk]

I've only glanced at the distribution-updater, but do you currently ignore builds triggered by PRs? If not, that's something we should probably do as well, isn't it?

[inukshuk]

Regarding the test suite, we should definitely make the output of failed test easier to understand; this will be helpful in and of itself and is also necessary if we want to be able to use the output to generate more helpful comments (so we should do that either way).

I suggest that we finish up a first iteration of the submission bot first (other than the templates itself we can still experiment with commit-based comments), and then work on improving the test suite output and adjusting the bot's comments. Does that sound good?

[rmzelle]

I don't know if it is possible to have different settings per hook

Ah, okay, it looks like it's not.

I suggest that we finish up a first iteration of the submission bot first

Sure. You just need text for https://github.com/inukshuk/Sheldon/tree/master/templates at this point?

[inukshuk]

Exactly. Depending on the information we want to be able to display, we may have to expose more locale variables to the template. Currently, the pull request template is passed the payload of GitHub's PR event and the two build events are passed the payload of the Travis notification.

[zuphilip]

Would it be possible and/or reasonable to automatically test, whether the url(s) for documentation are still alive?

[rmzelle]

@zuphilip, while good to check, that something we should ideally do for all the styles in the repository, not just for new submissions.

[zuphilip]

Yeah, "ideally"... However, I doubt that we can easily correct hundreds of missing documentation urls. Moreover, if the old documentation url is not working anymore, it may be a good idea to check whether the style is otherwise still up-to-date (and this can really take a long time). If we don't check the style overall, then we might end up with an style following an old (non-existing) documentation which is linked to the new documentation, which would be confusing. Not?

[rmzelle]

Sure.

[inukshuk]

@rmzelle @adam3smith here is a new example of a PR with a failing build that is then fixed with the next commit. The order of the commits and comments is currently independent of each other (because we are commenting on the PR and not on the commit), but I would think that this shouldn't be a problem in practice: in the example above the comments seem out of order, because I committed the fix before the failing build had finished.

[rmzelle]

Looks great. I would want to adjust the text a little, but otherwise this should already be very useful in its current form.

[adam3smith]

Yes, that's great, thanks!
Question: So, my dream scenario would be that the 2nd message would be customized according to the specific test failed. Is that at all realistic?

[inukshuk]

@rmzelle I set up a continuous deployment to Heroku, so we can simply change the templates and push to GitHub; if the corresponding Travis build succeeds, it will be pushed to Heroku automatically.

@adam3smith in this first version we expose only the payload of the Travis webhook to the template. This looks something like this -- so unfortunately we know nothing about which test failed exactly. Still, I'd suggest that we try to push this first version out the door finally to collect some real word experiences with the bot.

But there is room for improvement:

• We can try to improve the test output on Travis to make it easier to understand
• We could make the bot smarter; for failed builds it could check out the corresponding commit locally and run the test suite -- this would allow us to gather additional information which we could then expose to the template.

In order to achieve this, however, we need to make the bot multi-threaded (or multi-process) and we need file system access. That's not possible on the free Heroku account I set up so this would probably also mean that we install the bot on a Zotero server instead (and likely lose the continuous deployment).

[rmzelle]

I'm wondering if we can easily use images in GitHub comments. Just for kicks:

[inukshuk]

Images should be no problem. We could host them together with the bot (i.e., in the repository) so they would be updated together with the templates.

[rmzelle]

Allow customization of message depending on Travis errors: I'm thinking we'll create human-written instructions for the most common travis errors

I discovered that RSpec can generate multiple outputs, e.g. the current output that ends up in the Travis log, and JSON output. See https://relishapp.com/rspec/rspec-core/v/3-4/docs/command-line/format-option#multiple-formats-and-output-targets (rspec example_spec.rb --format progress --format documentation --out rspec.txt) and http://jing.io/t/programmatically-execute-rspec-and-capture-result.html (includes an example of what the JSON would look like).

If we can parse the JSON output with Sheldon, it should be possible to post the RSpec errors directly into the GitHub comments (in a concise format), so people won't have to visit the Travis build pages anymore.

[inukshuk]

We'd still need to run the tests with Sheldon (which is not feasible with the free Heroku plan, because we'd want at least two processes), because the Travis notification does not include any test results but only the build status.

[rmzelle]

Ah, okay, let's put this on the back-burner then.

We can't just have Sheldon extract the errors from the Travis logs with some regex, though? (see e.g. https://s3.amazonaws.com/archive.travis-ci.org/jobs/114556023/log.txt)

[inukshuk]

Right now Sheldon is notified by the GitHub or Travis hooks; these are simple HTTP requests which expect a response. Now, it should be possible to fetch the log file (another HTTP request) analyze it, compose the message, post it to GitHub (another HTTP request) and finally answer the web hook... but that's stretching it.

Ideally, we would have a very simple listener that accepts incoming web hooks and stores them temporarily and have a separate process or thread handle the requests in the background. This worker thread could easily take a little longer (e.g., examine the commit in detail, run tests, or fetch additional logs from Travis) because the web hook has already succeeded at this point. There are many possibilities, but I'd rather switch to such a listener/worker architecture before doing anything like that.

[rmzelle]

There are many possibilities, but I'd rather switch to such a listener/worker architecture before doing anything like that.

Is that for performance reasons, or just because the architecture would be easier to maintain?

[inukshuk]

The reason is that I'm worried the web service hooks might time-out (even though they work) if we take too long; I imagine GitHub and Travis CI don't appreciate it if takes a long time for their hooks to finish. The easiest solution would be to use a worker process (but we can't start multiple processes on Heroku for free); in our case we could get by with a second thread, but I have not tested if Heroku would let us do that on the free plan.