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

Release workflow #913

Merged
merged 7 commits into from
May 20, 2021
Merged

Release workflow #913

merged 7 commits into from
May 20, 2021

Conversation

Yoshanuikabundi
Copy link
Collaborator

As we discussed in #286, this PR adds a release workflow that uses a floating tag called latest to point to the latest release. In addition, it caches the release on DockerHub (addressing $416) and tells Binder to look there. Finally, it updates all links to Binder in the repository to point to the latest tag.

I think the workflow can be manually tested on an existing release?

@codecov
Copy link

codecov bot commented Apr 26, 2021

Codecov Report

Merging #913 (50ef020) into master (8248f5a) will increase coverage by 10.49%.
The diff coverage is n/a.

Copy link
Member

@j-wags j-wags left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update: This is blocked by me. secrets.DOCKER_USERNAME and secrets.DOCKER_PASSWORD are not currently populated, so this PR won't work until I get those.

@j-wags
Copy link
Member

j-wags commented May 12, 2021

@Yoshanuikabundi: I have access to our dockerhub username and password now. However, I started reviewing this PR, and I'm bothered by the use of high-value secrets in this repo (both because of the recent CodeCov leak, and in general because it prevents us from letting people submit PRs from forks). So I don't think we should add more secrets (like docker credentials) to this repo -- In fact, if we can get this repo down to zero secrets, it can be open to PRs from forks!

So, how hard would it be to move this automation to https://github.com/openforcefield/status/? That repo is more internal and there's no expectations of outside contributions to it.

Copy link
Member

@j-wags j-wags left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great, but I'd prefer to keep the dockerhub secrets out of this repo. So let's:

  • Remove the dockerhub build+upload from this PR
  • Keep everything else in this PR
  • Let's not worry about making/caching the docker images for now

@Yoshanuikabundi
Copy link
Collaborator Author

@j-wags I was a little worried about this PR so I haven't merged it yet. I made a test repo to see how it works. I did the following:

  1. Create the repo in GitHub with a readme
  2. Create a release v0.1.0 from the initial commit through GitHub
  3. Clone locally
  4. git tag latest && git push --tags
  5. Added and committed the Actions workflow
  6. Made a small change to the readme, committed it, and pushed
  7. Published a new release v0.2.0 through GitHub

As I feared, the latest tag shows up as a release. But the workflow does work at least, and within about a minute after the v0.2.0 release was published the tag had updated.

We could get around that minor ugliness by using a branch rather than a tag with this action. That would have the downside that it would be possible for someone to accidentally check out the latest branch and commit to it, which can't happen with a tag.

Just wanted to bring that up before I merge this PR and we're stuck with a weird pseudo-release forever. Which do you prefer? Sorry, I should've brought this up at our check in but it must've slipped my mind.

Copy link
Member

@j-wags j-wags left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for checking into this, @Yoshanuikabundi. I think it's fine to have a latest tag, since it doesn't move to the top of the Releases list, even when it gets updated.

However, I'd like to keep this manual for now, so I've proposed that change. Let's also make sure to add this to the standard release process when we merge.

.github/workflows/release.yml Outdated Show resolved Hide resolved
.github/workflows/release.yml Outdated Show resolved Hide resolved
@j-wags
Copy link
Member

j-wags commented May 20, 2021

Sorry for the flaky tests -- Merging!

@j-wags j-wags merged commit fa851e6 into master May 20, 2021
@j-wags j-wags deleted the release-workflow branch May 20, 2021 22:56
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.

2 participants