-
Notifications
You must be signed in to change notification settings - Fork 168
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add AppVeyor pull request previews #262
Conversation
AppVeyor will not skip the build unless it appears here https://www.appveyor.com/docs/how-to/filtering-commits/#skip-directive-in-commit-message
Intentionally keep conda activate outside script per conda/conda#7980
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! Will review more and add additional comments.
.appveyor.yml
Outdated
# See https://www.appveyor.com/docs/getting-started-with-appveyor-for-linux/ | ||
skip_branch_with_pr: true | ||
|
||
# AppVeyor will run on the output branch commits until the commit message |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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]"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant "unless". Fixed and expanded in 0817820
Activating AppVeyor for this repositoryHere are the steps I followed to activate AppVeyor for this repo.
@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: |
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
That's right, if I recall correctly from past projects with my |
✅ Build rootstock 1.0.7 completed (commit 4430796d92 by @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). |
✅ Build rootstock 1.0.8 completed (commit e141721e1b by @agitter) |
Seems like whitelisting the following directories would be sufficient: I'll get started on the manubot CI environment detection implementation: manubot/manubot#130. |
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. |
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.
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 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. |
Thanks @rhagenson, outside opinions are always welcome. We want to make sure Manubot is useful to more people than the core team.
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. |
Temporary, for help with manubot/manubot#130
✅ Build rootstock 1.0.9 completed (commit a5585c4e88 by @dhimmel) |
@agitter I committed 4aec0e2 to output the list of environment variables for help with manubot/manubot#131. AppVeyor did not run even though As per these docs, we could also add:
This would allow users to force the appveyor build via commit message in case their whitelisting rules are too stringent. |
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., 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. |
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 |
AppVeyor build 1.0.14 for failed. |
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 |
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. |
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. |
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 |
AppVeyor build 1.0.18 failed. . |
AppVeyor build 1.0.19 for commit 05244d0 by @agitter failed. The rendered manuscript from this build is temporarily available for download at |
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 |
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 |
@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. |
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. |
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @agitter on this fairly large enhancement with a quite simple diff!
This build is based on bc094bd. This commit was created by the following Travis CI build and job: https://travis-ci.com/manubot/rootstock/builds/122622628 https://travis-ci.com/manubot/rootstock/jobs/223977466 The full commit message that triggered this build is copied below: Add AppVeyor pull request previews Merges #262 References #198 Add an AppVeyor continuous integration file to enable manuscript build previews in pull requests. AppVeyorBot comments in pull requests with a link to the latest manuscript PDF. Move shared continuous integration steps to an install script. Change the continuous integration skip message in the deploy script to work across multiple continuous integration services.
This build is based on bc094bd. This commit was created by the following Travis CI build and job: https://travis-ci.com/manubot/rootstock/builds/122622628 https://travis-ci.com/manubot/rootstock/jobs/223977466 The full commit message that triggered this build is copied below: Add AppVeyor pull request previews Merges #262 References #198 Add an AppVeyor continuous integration file to enable manuscript build previews in pull requests. AppVeyorBot comments in pull requests with a link to the latest manuscript PDF. Move shared continuous integration steps to an install script. Change the continuous integration skip message in the deploy script to work across multiple continuous integration services.
Merges manubot/rootstock#262 References manubot/rootstock#198 Add an AppVeyor continuous integration file to enable manuscript build previews in pull requests. AppVeyorBot comments in pull requests with a link to the latest manuscript PDF. Move shared continuous integration steps to an install script. Change the continuous integration skip message in the deploy script to work across multiple continuous integration services.
Merges manubot/rootstock#262 References manubot/rootstock#198 Add an AppVeyor continuous integration file to enable manuscript build previews in pull requests. AppVeyorBot comments in pull requests with a link to the latest manuscript PDF. Move shared continuous integration steps to an install script. Change the continuous integration skip message in the deploy script to work across multiple continuous integration services.
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?