Add AppVeyor pull request previews

[agitter]

Partially addresses #198. Tested using https://github.com/agitter/my-manuscript/. See agitter/my-manuscript#9 (comment) for an example pull request notification.

I still need to get the shared Travis CI and AppVeyor setup to work from a script.

@dhimmel do I have permissions to enable AppVeyor on this repository? Or can you please do it?

[agitter]

The Travis CI build now works with an install script. Once we enable AppVeyor for this repository and test that build, this is ready for review.

[dhimmel]

Great! Will review more and add additional comments.

[dhimmel]

Do you mean "unless" rather than "until"? Or are you saying CI runs on the initial empty commit like 13115af? It seems like our initial commits do include [ci skip]:

git commit --allow-empty \
  --message "Initialize empty branch" \
  --message "[ci skip]"

[agitter]

I meant "unless". Fixed and expanded in 0817820

[dhimmel]

Activating AppVeyor for this repository

Here are the steps I followed to activate AppVeyor for this repo.

1. Go to https://ci.appveyor.com/projects/new
2. Click "update installations"
3. for "Where do you want to install AppVeyor?" chose "configure" the manubot GitHub organization. If we didn't already have the organization linked, I think it would say "Request"
4. Only select repositories... choose manubot/rootstock
5. Click on +Add after hovering over manubot/rootstock.
6. CI is activated at https://ci.appveyor.com/project/manubot/rootstock. Note that I believe manubot in this URL does not refer to the github manubot organization, but instead our account on appveyor.

@agitter do you have control over the CI? I don't know if the best way is to share the login info for the manubot appveyor account or what? Ah https://ci.appveyor.com/team looks promising:

[agitter]

do you have control over the CI?

I believe I have access to the Manubot team now. I can access its AppVeyor account but don't see myself listed as a member at https://ci.appveyor.com/account/manubot/team

I sent myself another invite using my email address associated with my AppVeyor account, and it said I had already joined the team. Seems like it worked.

Note that I believe manubot in this URL does not refer to the github manubot organization, but instead our account on appveyor.

That's right, if I recall correctly from past projects with my gitter-lab organization.

[AppVeyorBot]

✅ Build rootstock 1.0.7 completed (commit 4430796d92 by @agitter)
Generated manuscript-1.0.7-4430796.pdf for review

[agitter]

I made a test commit (now reverted) that changed the content to trigger an AppVeyor build. That worked as expected.

We still need to decide whether to build all commits, only those that affect whitelisted files, or all commits except those that only affect blacklisted files (e.g. README.md).

[AppVeyorBot]

✅ Build rootstock 1.0.8 completed (commit e141721e1b by @agitter)
Generated manuscript-1.0.8-e141721.pdf for review

[dhimmel]

We still need to decide whether to build all commits, only those that affect whitelisted files, or all commits except those that only affect blacklisted files (e.g. README.md).

Seems like whitelisting the following directories would be sufficient: content, build, ci. Also do we need to whitelist .appveyor.yml or is that assumed?

I'll get started on the manubot CI environment detection implementation: manubot/manubot#130.

[rhagenson]

If I can add my two-cents as an outsider and fan of this project, setting up AppVeyor previews appears to be intended to build a preview of pull requests prior to acceptance where after the preview effectively becomes the content on gh-pages. Am I mistaken on this point?

Building on all PRs would then maintain consistency in the PR review process, as well as eliminate all possible mistakes in whitelist/blacklist-ing files properly as a project expands. In my opinion, the one submitting a PR should not need to know what is whitelisted or blacklisted in order for their PR to be reviewed the same as any other.

[dhimmel]

Hey @rhagenson, thanks for your input. Your understanding is correct, the AppVeyor previews allow viewing the PDFs that commits in a pull request create before having merged the PR.

Building on all PRs would then maintain consistency in the PR review process, as well as eliminate all possible mistakes in whitelist/blacklist-ing files properly as a project expands.

Yes, however it could lead to cluttering PRs with AppVeyor comments and notification fatigue. For example, if someone creates a PR to update a README, then every commit in the PR will trigger a comment like #262 (comment). IMO it'd be best to avoid these AppVeyor comments unless there has been an actual change. An alternative solution would be for the user to put [skip ci] in their commit message, although it's easy to forget this.

I think that the whitelist approach is pretty safe in terms of not building commits that actually effect the manuscript, but let's see what @agitter thinks is most prudent.

[agitter]

Thanks @rhagenson, outside opinions are always welcome. We want to make sure Manubot is useful to more people than the core team.

setting up AppVeyor previews appears to be intended to build a preview of pull requests prior to acceptance

Yes, that's right. Ideally we could have done this within the Travis CI builds, but Travis CI does not support persistent artifacts like AppVeyor, Circle CI, etc. The point is indeed to help with pull request review. Currently, it is hard for a reviewer to know whether a reference's metadata will be extracted as expected unless they manually check via the Manubot command line interface or something similar.

I see the case for building on all commits to keep things simple. The only real downside would be excessive notifications in projects that frequently modify files that do not affect the manuscript. For instance, https://github.com/Benjamin-Lee/deep-rules often has pull requests to change the contributor tracking table that do not affect the manuscript or build process.

[AppVeyorBot]

✅ Build rootstock 1.0.9 completed (commit a5585c4e88 by @dhimmel)
Generated manuscript-1.0.9-a5585c4.pdf for review

[dhimmel]

@agitter I committed 4aec0e2 to output the list of environment variables for help with manubot/manubot#131. AppVeyor did not run even though .appveyor.yml was edited, so looks like we have it explicitly whitelist it as I do in 1e8d10d.

As per these docs, we could also add:

only_commits:
  message: /\[force ci\]/

This would allow users to force the appveyor build via commit message in case their whitelisting rules are too stringent.

[rhagenson]

I understand the desire to avoid notification fatigue. In that case, I think the whitelist approach is best with the addition of clearly documenting that only edits under specific directories (e.g., content, build, and ci, as previously mentioned) trigger previews.

Not too familiar with the manubot/rootstock build process (been following it, but not yet used it) so as long as there is no way to leak content into other directories that would then need whitelisting and complicate the build process I see no reason why whitelisting could not be used. Ultimately, my opinion on this is driven by this repo being the initial template others use so any decision forces a process upon potential users -- the less complex the forced process, the more likely users are to continue developing with manubot.

[AppVeyorBot]

AppVeyor build 1.0.13 for commit 203fb23 by @agitter is now complete. The rendered manuscript from this build is temporarily available for download at manuscript-1.0.13-203fb23.pdf.

[AppVeyorBot]

AppVeyor build 1.0.14 for failed.

[AppVeyorBot]

AppVeyor build 1.0.16 for commit 3d1f703 by @agitter is now complete. The rendered manuscript from this build is temporarily available for download at manuscript-1.0.16-3d1f703.pdf.

[dhimmel]

Sometimes a build may fail but a PDF gets created nonetheless. It can be helpful to see the PDF to diagnose the build failure sometimes (like if its citation related). Would there be any way to link to the manuscript artifact for failures if it exists?

Also I am okay with continuing to refine the AppVeyor setup in subsequent PRs, if we'd like to get this merged sooner.

[agitter]

Would there be any way to link to the manuscript artifact for failures if it exists?

Yes, that should be possible. I'm testing builds under a few different conditions so we can see if the notifications work as expected under these different scenarios.

[AppVeyorBot]

AppVeyor build 1.0.17 for commit 091454c by @agitter is now complete. The rendered manuscript from this build is temporarily available for download at manuscript-1.0.17-091454c.pdf.

[AppVeyorBot]

AppVeyor build 1.0.18 failed. .

[AppVeyorBot]

AppVeyor build 1.0.19 for commit 05244d0 by @agitter failed. The rendered manuscript from this build is temporarily available for download at manuscript-1.0.19-05244d0.pdf.

[AppVeyorBot]

AppVeyor build 1.0.20 for commit 130abba by @agitter is now complete. The rendered manuscript from this build is temporarily available for download at manuscript-1.0.20-130abba.pdf The rendered manuscript from this build is temporarily available for download at duplicate-manuscript-1.0.20-130abba.pdf.

[AppVeyorBot]

AppVeyor build 1.0.21 for commit a8f0dc8 by @agitter is now complete. The rendered manuscript from this build is temporarily available for download at manuscript-1.0.21-a8f0dc8.pdf.

[agitter]

@dhimmel these notification messages are good enough for now. We can see if we want to revise them once we start using them in real workflows.

I'd like to review once more end to end before merging to make sure I didn't leave in any test code, but I'm not planning any more updates. I'm also okay waiting for manubot/manubot#131.

[agitter]

This is ready for final review and merge. I suggest we make a separate pull request to follow up on manubot/manubot#131 that updates the front matter and Manubot version.

When we merge, I'd also like to remove the URL in the commit message of 70c86f2. It's no longer relevant so we don't need to spam that linked issue.

[AppVeyorBot]

AppVeyor build 1.0.22 for commit 808d425 by @agitter is now complete. The rendered manuscript from this build is temporarily available for download at manuscript-1.0.22-808d425.pdf.

[dhimmel]

Thanks @agitter on this fairly large enhancement with a quite simple diff!