Exposing rendered manuscripts from pull request builds

[dhimmel]

Currently, rendered manuscripts created by pull request builds, such as output/manuscript.html and output/manuscript.pdf, are never exposed. Instead they disappear with the ephemeral build environment.

Exposing rendered manuscripts (and ideally a rendered diff, see #54) would make review of pull requests easier. For example, currently we don't know how a citation will render until we merge the PR (or test it locally). Nor do we know if formatting syntax will work as intended. The point has also been brought up by a reviewer of the manubot manuscript in greenelab/meta-review#121.

The main issue blocking us is that pull requests on Travis cannot access secure environment variables due to their unauthenticated nature. This limitation is discussed at travis-ci/travis-ci#5579. Possibly there are workarounds or alternatives to using PR build artifacts.

[agitter]

To learn more about our options, should we test an alternative CI service? CircleCI supports artifacts. That wouldn't provide the complete solution we are looking for. It may provide a quick short term workaround to the specific problem of reviewing rendered manuscripts for a PR.

[dhimmel]

I would be interested to see what an operational .circleci/config.yml looks like, so we can evaluate how it compares to Travis. However, switching CI providers would be highly disruptive, so I'm cautious to have us invest too much time in trying out Travis alternatives, until we have a roadmap that accommodates CI migration. Certain designs may also allow us to support multiple CI providers.

[agitter]

I tested building a manuscript with AppVeyor, which supports build artifacts in pull requests, to assess how much that would help. agitter/my-manuscript#4 provides a rough demonstration.

The overall strategy is to continue using Travis CI for deployment and use a parallel build on AppVeyor to store a copy of the updated output/manuscript.pdf from the pull request. The build process in the AppVeyor config file is fairly similar to the .travis.yml file. The AppVeyor build writes the URL of the latest PDF:

Updated PDF available from https://ci.appveyor.com/api/projects/agitter/my-manuscript/artifacts/output/manuscript.pdf?branch=add-ref

I haven't tested whether this works if the pull request originates from a different GitHub fork.

@dhimmel do you think I should continue exploring this option or the equivalent with CircleCI? If we add .appveyor.yml to rootstock, by default it would be ignored. We could give instructions to activate AppVeyor CI on the manuscript repository to gain access to the AppVeyor build artifacts. No other setup should be required.

Storing a PDF of a rendered diff would work similarly. We would only need to extend the build script to generate it.

[dhimmel]

The overall strategy is to continue using Travis CI for deployment and use a parallel build on AppVeyor to store a copy of the updated output/manuscript.pdf from the pull request.

It is not ideal to have to use two CI services like this, but it is the best solution we have so far for build artifacts. I am on board with making it an optional step in SETUP.md. We may want to pull out duplicate code in build.sh to scripts. We also may want to see if appveyor.yml could go in ci to avoid cluttering the root directory, although that could be confusing with .travis.yml being elsewhere.

The other option is to switch to GitLab, but I think that could diminish project visibility and broad participation in manuscripts. On the other hand, it's closer to open source and has several design improvements over GitHub. But is also lacks important features like suggestions in reviews. (it has suggestions)

[agitter]

I could pull out most of the duplicate install stage steps into a script. AppVeyor doesn't have a separate before_install stage. Do we care whether the conda environment creation is run in the before_install stage in the Travis CI build?

I believe we would need to keep the conda activate manubot step out of the script. I have run into major problems trying to activate newer versions of conda inside a script.

Moving the AppVeyor config file to the ci subdirectory would require modifying the AppVeyor project settings per these docs. That would be cleaner, but I don't think it is worth the extra configuration step.

Another related note is the Manubot currently only adds CI build metadata for Travis CI builds. We could generalize that for multiple CI services. That would be very low priority and may not be necessary.

We may want to eventually switch to GitLab. That decision would have more widespread impact so I'd like to do more internal testing first. Ideally we could give Manubot users the choice of GitHub or GitLab, but we would need to consider how much that increases our maintenance burden.

[dhimmel]

Do we care whether the conda environment creation is run in the before_install stage in the Travis CI build?

No I don't think so. I haven't figured out meaningful differences between install and before_install besides potentially for some install-specific behavior that we don't use (source):

By default Travis CI uses pip to manage Python dependencies. If you have a requirements.txt file, Travis CI runs pip install -r requirements.txt during the install phase of the build.
You can manually override this default install phase, for example:

install: pip install --user -r requirements.txt

I believe we would need to keep the conda activate manubot step out of the script.

Yes, some duplication should be fine. It would be nice to keep things miniconda version to a single script.

Another related note is the Manubot currently only adds CI build metadata for Travis CI builds. We could generalize that for multiple CI services.

Yes this is a good idea that I can do if we go forward with AppVeyor: the artifacts combined with notifications makes me think we should. One weird aspect IIRC is that appveyor does not use the github organization for its URLs but instead uses username from a separate account.

If the purpose of AppVeyor is only artifacts / notifications on PRs, should we see if we can have it run only on PRs?

[dhimmel]

If the purpose of AppVeyor is only artifacts / notifications on PRs, should we see if we can have it run only on PRs?

Checking the Do not build on "Push" events setting may have this effect... not sure if we can put that in .appveyor.yml.

@agitter seems like we should move forward with a rootstock pull request with your solution.

[agitter]

One weird aspect IIRC is that appveyor does not use the github organization for its URLs but instead uses username from a separate account.

Yes, that sounds familiar. I believe you figured out how to set that up for the manubot/manubot repo.

Do not build on “Push” events is one of the settings that is only available through the UI per this doc. I haven't been able to find a good solution to only run AppVeyor on PRs. In agitter/my-manuscript#8 I tried a regex to only run on the automatic merge commits made on the master branch, but the commit message shown on GitHub is not what CI services see.

AppVeyor also runs on the output branch commits from the Travis CI deployments. The only way to disable this is through the project settings UI. I believe that is because those output branch commits do not have a .appveyor.yml file.

Despite these limitations, I'll move ahead with a rootstock pull request in the next few days.

[dhimmel]

AppVeyor also runs on the output branch commits from the Travis CI deployments.

But our commit messages on the output branch include [ci skip]... shouldn't this suppress CI?

I haven't been able to find a good solution to only run AppVeyor on PRs.

Does the Do not build on “Push” events setting not have this effect? The more I think about it, I think some repos like rootstock actually probably want to run AppVeyor on master to ensure it's not broken. But other repos may want to keep things clean and avoid it.

[agitter]

Per the these docs

AppVeyor searches for [skip ci] / [ci skip] / [skip appveyor] in the commit message title only. This avoids confusion when squashing commits, in particular with the GitHub squash-and-merge feature.

Do we want to append [ci skip] to the commit message title instead of adding it later in the commit message? I'm mildly in favor of that. I don't think many people read the output branch commit messages.

Does the Do not build on “Push” events setting not have this effect?

Yes, that should work, but I haven't tested it. I meant that I hadn't found a way to only run AppVeyor on pull requests through the .appveyor.yml file alone.

[agitter]

I tested Do not build on “Push” events and it does work as expected. We can document how to set this in the AppVeyor settings if a user only wants it for pull request review. However, as you noted, it wouldn't be bad to let AppVeyor build everything on master.

[dhimmel]

I am going to close this issue now that we have AppVeyor configured to post artifacts of the manuscript PDF. Further discussion can happen below or in new issues.

Also linking a Twitter thread on whether GitHub Actions would support PR artifacts. We don't have a conclusive indication at the moment.

[dhimmel]

Also linking a Twitter thread on whether GitHub Actions would support PR artifacts. We don't have a conclusive indication at the moment.

See https://github.com/actions/upload-artifact

[leehart]

Please forgive the crudity of my method, but I took the route of creating an alternative workflow step that simply deploys the "webpage" artifacts from the PR to a subdirectory of the gh-pages branch, which then gets served automatically, e.g. https://owner.github.io/paper/PR/8. Of course, it would be convenient if something similar was done out-of-the-box for PR builds, and I suspect a more elegant solution is already in the pipe, or perhaps something already exists for GitHub Actions? Advice very welcome!

[dhimmel]

I took the route of creating an alternative workflow step that simply deploys the "webpage" artifacts from the PR to a subdirectory of the gh-pages branch

That's a cool approach. Is your manuscript public so we could see how you did it? It might make sense to tweak the directory structure a bit like /v/pr/8 or /v/pr8, since we already use the v directory for versioned manuscripts. Another possibility is to deploy the manuscript to the /v/commit directory and then update /v/pr/8, which would be a synlink pointing to /v/commit.

The main problem with this approach is that it will only work for PRs from users with write access I believe (or maybe PRs from a branch in the same repo). If an external user tries to make a pull request from a branch on their fork, this will fail... which might be okay, but just it won't work for all PRs.

perhaps something already exists for GitHub Actions

we do upload artifacts for GitHub Actions that contain the rendered manuscript... just they're annoying to access because you have to go the actions page and click artifacts.

[leehart]

Great pointers! Many thanks @dhimmel

That's a cool approach. Is your manuscript public so we could see how you did it?

Proof of concept:
https://github.com/malariagen/ag1000g-phase3-data-paper/pull/9/files