-
Notifications
You must be signed in to change notification settings - Fork 182
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
GitHub Actions workflow for building and deployment #297
Conversation
to test github actions
Based on the in-progress GitHub Actions workflow in manubot/rootstock#297
merges #4 closes #3 Based on the in-progress GitHub Actions workflow in manubot/rootstock#297
github.event_name == 'push' && | ||
job.status == 'success') | ||
env: | ||
MANUBOT_SSH_PRIVATE_KEY: ${{ secrets.MANUBOT_SSH_PRIVATE_KEY }} |
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.
perhaps we can also test if secrets.MANUBOT_SSH_PRIVATE_KEY
is set and if not, provide a better error 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.
Yes, better error messages would be valuable. Setting up CI has been a common source of errors.
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.
Okay I will save this for a subsequent PR. Possibly that also removes the legacy travis deploy key method of deployment.
This action is alive and working (including deployment) at:
One question is whether we want to switch to GitHub Actions as the primary method CI method for rootstock and future clones. I think we probably do, since it reduces the setup from using two services (GitHub and Travis) to just one (GitHub). It's also more aligned with the longterm roadmap. Unfortunately, we still have to manually generate a deploy key to push to GitHub until since One option if we switch to actions as the primary CI going forward, is that we can add something like the following to the cleanup step of setup: git rm .travis.yml .appveyor.yml ci/install.sh Or do we want to remove this files from rootstock, thereby nudging existing users to switch? |
@@ -0,0 +1,40 @@ | |||
name: Manubot |
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.
Maybe this could have a more descriptive name, since the user's might theoretically poke around and see it?
Maybe Manubot Continuous Integration
or Manubot CI
or just Manubot Build and Deploy Manuscript
Maybe the file name could be manubot-ci.yml
or manubot-build-and-deploy.yml
Finally, is "manubot" necessary in these names since we're already in the context of manubot, being in a manubot repository.
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.
is "manubot" necessary in these names since we're already in the context of manubot
I am thinking about the situation where someone creates a repository with a manuscript as well as other stuff. The other stuff may need additional workflows. Those other workflows may have nothing to do with Manubot... for example perhaps one runs an R analysis, another one spellchecks pull requests, another makes sure PR authors have signed a contributor license agreement, another one automatically merges approved PRs. In these cases, I envision you want workflow names that best differentiate the specific workflow.
Since the name "Manubot" is used in the context of a workflow, I think adding CI
or "Continuous Integration" afterwards is a bit redundant. My understanding is that workflows are basically CI, and perhaps GitHub wanted to get away from the CI terminology because it's a bit confusing.
One final point is that I see the GitHub action workflow as become the main way people run manubot. The rest of the manuscript repo will be their content / config files. The action is actually "the manubot", or at least the place where "the manubot" runs.
@@ -0,0 +1,40 @@ | |||
name: Manubot |
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.
Maybe you want to add a status badge to the readme:
![](https://github.com/manubot/rootstock/workflows/manubot/badge.svg)
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.
Oh wait, that wont refer to the specific manuscript. Would a relative image link work? ![](workflows/manubot/badge.svg)
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.
The relative image link won't work, first because it's an SVG and second because that is not a file path relative to the repo content.
We could use the absolute URL. It would get rebranded to the manubot instance from manubot/rootstock
during setup. Do you think badges are important given that you can now see the check or fail mark from the main page? I guess it makes it more clear for those unfamiliar with CI / GitHub actions?
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 do think that badges are more obvious to a larger audience that may not know to look for the pass/fail icons.
.github/workflows/manubot.yaml
Outdated
- master | ||
jobs: | ||
build: | ||
name: Build Manuscript |
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.
Maybe Build and Deploy Manuscript
to be more accurate, and to avoid duplicate name of the Build Manuscript
step below.
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.
FWIW I think you can omit this and in the Actions
tab it will just show up as the config file name at the top.
.github/workflows/manubot.yaml
Outdated
with: | ||
name: output | ||
path: output | ||
- name: Deploy |
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.
Maybe Deploy Manuscript
to keep in line with the [verb] [noun]
pattern of the other step names above.
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.
Yes, I like this suggestion too
AppVeyor build 1.0.86 for commit 70398be by @dhimmel 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.
These changes look good to me, and I support the decision to make GitHub Actions the default but leave optional instructions for using Travis CI.
In addition to my small comments, there are a few open comments from @vincerubinetti we can resolve before merging.
GITHUB_PULL_REQUEST_SHA: ${{ github.event.pull_request.head.sha }} | ||
shell: bash --login {0} | ||
run: bash build/build.sh | ||
- name: Upload Artifacts |
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.
Will this eventually replace the AppVeyor artifacts and notification messages?
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.
The main difference now is that it doesn't provide notification messages (possibly it could not sure). Although I'm not sure I love the messages since they get overwhelming.
So yeah, I think it is a satisfactory substitute, but requires users are aware of the feature, since you have to navigate to it.
@@ -4,7 +4,6 @@ | |||
|
|||
[![HTML Manuscript](https://img.shields.io/badge/manuscript-HTML-blue.svg)](https://manubot.github.io/rootstock/) | |||
[![PDF Manuscript](https://img.shields.io/badge/manuscript-PDF-blue.svg)](https://manubot.github.io/rootstock/manuscript.pdf) | |||
[![Build Status](https://travis-ci.com/manubot/rootstock.svg?branch=master)](https://travis-ci.com/manubot/rootstock) |
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 like the suggestion to include a GitHub Actions badge here. Because we can't adaptively show the Travis or GitHub Actions badge based on which CI service is being used, would we default to GitHub's?
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.
SETUP.md
Outdated
|
||
GitHub Actions is the default service for new manucripts since it's easiest to setup. | ||
We recommend using either GitHub Actions or Travis CI, but not both to avoid deploying manuscripts multiple times. | ||
AppVeyor can be used in addition to GitHub Actions or Travis CI to comment on pull request with download links to renderred PDFs. |
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.
"renderred" -> "rendered" here and in the next line.
SETUP.md
Outdated
# Print the URL for adding the private key to GitHub | ||
echo "https://github.com/$OWNER/$REPO/settings/secrets" | ||
|
||
# Print the encoded private key for copy-pasting to Travis CI |
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.
"Travis CI" -> "GitHub"
|
||
### GitHub Actions | ||
|
||
If you plan on only using GitHub actions, you can remove configuration files for other CI services: |
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 think this is fine for now instead of removing the files from rootstock. Some users may prefer to continue using Travis CI for reasons we can't anticipate.
Check the "Allow write access" box below. | ||
Finally, click "Add key". | ||
|
||
#### Add the private key to Travis CI | ||
#### Add the private key to GitHub |
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 didn't test these steps, but they are easy to understand.
Switch to GitHub actions as the primary CI service for new manuscripts, while retaining backwards compatibility with existing Travis manuscripts.
Todo: