Skip to content
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

Merged
merged 30 commits into from
Aug 9, 2019
Merged

Conversation

agitter
Copy link
Member

@agitter agitter commented Aug 3, 2019

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
Copy link
Member Author

agitter commented Aug 4, 2019

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.

Copy link
Member

@dhimmel dhimmel left a 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
Copy link
Member

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]"

Copy link
Member Author

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

.appveyor.yml Show resolved Hide resolved
.appveyor.yml Show resolved Hide resolved
ci/deploy.sh Show resolved Hide resolved
@dhimmel
Copy link
Member

dhimmel commented Aug 4, 2019

Activating AppVeyor for this repository

Build status

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:

image

@agitter
Copy link
Member Author

agitter commented Aug 4, 2019

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
image
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.

@agitter agitter changed the title [WIP] Add AppVeyor pull request previews Add AppVeyor pull request previews Aug 6, 2019
@AppVeyorBot
Copy link

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

@agitter
Copy link
Member Author

agitter commented Aug 6, 2019

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
Copy link

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

@dhimmel
Copy link
Member

dhimmel commented Aug 6, 2019

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
Copy link
Contributor

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
Copy link
Member

dhimmel commented Aug 6, 2019

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
Copy link
Member Author

agitter commented Aug 6, 2019

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
Copy link

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

@dhimmel
Copy link
Member

dhimmel commented Aug 6, 2019

@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
Copy link
Contributor

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
Copy link

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
Copy link

AppVeyor build 1.0.14 for failed.

@AppVeyorBot
Copy link

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
Copy link
Member

dhimmel commented Aug 8, 2019

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
Copy link
Member Author

agitter commented Aug 8, 2019

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
Copy link

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
Copy link

AppVeyor build 1.0.18 failed. .

@AppVeyorBot
Copy link

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
Copy link

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
Copy link

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
Copy link
Member Author

agitter commented Aug 8, 2019

@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
Copy link
Member Author

agitter commented Aug 9, 2019

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
Copy link

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.

Copy link
Member

@dhimmel dhimmel left a 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!

@agitter agitter merged commit bc094bd into manubot:master Aug 9, 2019
@agitter agitter deleted the appveyor branch August 9, 2019 16:41
dhimmel pushed a commit that referenced this pull request Aug 9, 2019
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.
dhimmel pushed a commit that referenced this pull request Aug 9, 2019
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.
adebali pushed a commit to CompGenomeLab/lemur-manuscript-archive that referenced this pull request Mar 4, 2020
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.
ploegieku added a commit to ploegieku/2023-functional-homology-paper that referenced this pull request Aug 6, 2024
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants