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

[MAINT] Add workflow to autodeploy to PyPi #568

Merged
merged 4 commits into from
Aug 21, 2020

Conversation

tsalo
Copy link
Member

@tsalo tsalo commented May 10, 2020

Closes None. This PR should make it much easier to make releases on PyPi. New tagged releases should just deploy automatically.

Changes proposed in this pull request:

  • Add a workflow to the CircleCI config to deploy to PyPi on tagged releases.

EDIT: @emdupre has added me as a maintainer for tedana on pypi.org, and I've added my username and password as environmental variables in the tedana CircleCI project settings, so the non-code end of things should be handled.

@tsalo tsalo requested a review from emdupre May 10, 2020 23:45
- merge_coverage
filters:
tags:
only: /[0-9]+(\.[0-9a-zA-Z]+)*/
Copy link
Member Author

Choose a reason for hiding this comment

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

The only thing I'm not 100% sure about is the chosen regex for our tags. In NiMARE and PyMARE, we're using the simpler /[0-9]+(\.[0-9]+)*/, but I didn't think that would catch the alpha and rc releases we sometimes make.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we'd have to decide on a naming convention for us to make a foolproof regex, so maybe we can talk about it on the call?
I tested this and it seemed pretty reasonable for anything I could think of.

Copy link
Member Author

Choose a reason for hiding this comment

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

@emdupre can correct me, but I believe our current working standard is to add a[0-9] (for alpha releases) or rc[0-9] (for release candidates) on those occasions we don't have regular releases. That said, I agree that we can/should talk about it on the call.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, let's discuss today ! But in general I'd prefer alpha or rc, rather than both... But I'm guilty of having used both so far, so we should definitely standardize ! Another reason to automate :)

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've opened #570 for the vote.

Copy link
Collaborator

Choose a reason for hiding this comment

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

With our decision about the rc convention, could we modify this regex?

@codecov
Copy link

codecov bot commented May 11, 2020

Codecov Report

Merging #568 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #568   +/-   ##
=======================================
  Coverage   92.86%   92.86%           
=======================================
  Files          23       23           
  Lines        1920     1920           
=======================================
  Hits         1783     1783           
  Misses        137      137           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d338acc...beb1f9d. Read the comment docs.

@tsalo
Copy link
Member Author

tsalo commented May 12, 2020

One other thing I should note is that it looks like a maintainer needs to make the release, rather than using release-drafter. I just went through the process with NiMARE and when I used the release-drafter draft it wouldn't trigger the deploy workflow in CircleCI. I had to delete the release and tag, and then draft a release from scratch myself. I think we can copy the draft contents from release drafter and then draft the release ourselves, but it's easy to forget to do that.

@smoia
Copy link
Collaborator

smoia commented May 15, 2020

Would you be interested in a solution to automatise the process to cut a release as well?

@tsalo
Copy link
Member Author

tsalo commented May 15, 2020

In what way?

@smoia
Copy link
Collaborator

smoia commented May 15, 2020

For instance, I recently set up auto for another project in this PR.
Auto creates automatically a release at every PR, preparing the release file and the log. Matched with your workflow that automatically creates a pypi package every time that there's a release cut, you would have a continuous flow.
It has nice functionalities that let cut master and development releases in parallel. It's originally based on labels, but I think it also supports conventional commits.
There are also other options out there, some specifically built for python!

@tsalo
Copy link
Member Author

tsalo commented May 15, 2020

Per our call today, @smoia will open an issue for auto specifically. Regarding the proposed approach here, I talked a bit with @smoia on GitHub and we are both concerned that this method may limit who can make releases.

I have adopted the method in NiMARE, and I noticed recently that releasing a draft made by release-drafter failed to trigger the deploy workflow. Also, since CircleCI includes my PyPi username and password as environmental variables, I wonder if releases must be made by me specifically. If that's the case, then that might be a problem with this method.

Alternatively, @smoia is using the pypa/gh-action-pypi-publish GitHub action in phys2bids, which should make every release happen using his account, regardless of who makes the release. Last year, I tried using the same action in NiMARE, but it caused problems on my forks. Unfortunately, I didn't do a very good job of documenting said problems, so I don't even know what the issue was or it is now solved. We can wait until phys2bids adopts their new approach to see if they come up against any problems, if folks would like. Or we could merge this and test whether the release authorship issue is actually a problem. Or something else entirely.

@tsalo
Copy link
Member Author

tsalo commented May 18, 2020

To check whether it's a problem if someone other than me makes the release, I propose that we move forward with these changes (pending review of course) and simply have someone else make the next release. If it fails to deploy, then we can always delete the release and associated tag, fix the deployment, and release again.

@smoia
Copy link
Collaborator

smoia commented Jul 17, 2020

I saw it was discussed in the meeting, this is our automatic PyPI deplyment workflow. It seems to work fine!

Note that I still cut the release manually (albeit using auto to set it up).
Once we'll have the auto workflow working properly in Travis CI, I'll check this works too!

- merge_coverage
filters:
tags:
only: /[0-9]+(\.[0-9a-zA-Z]+)*/
Copy link
Collaborator

Choose a reason for hiding this comment

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

With our decision about the rc convention, could we modify this regex?

@tsalo
Copy link
Member Author

tsalo commented Aug 1, 2020

With our decision about the rc convention, could we modify this regex?

@jbteves I'm not great with regular expressions. Any thoughts on what the updated one should look like?

@jbteves
Copy link
Collaborator

jbteves commented Aug 1, 2020

Well I feel awkward approving this since I contributed the regex.
@emdupre @handwerkerd any chance you could take a look?

@jbteves jbteves self-requested a review August 1, 2020 15:59
@jbteves jbteves dismissed their stale review August 1, 2020 16:00

It's my own review. I contributed the regex, so I shouldn't approve.

Copy link
Member

@emdupre emdupre left a comment

Choose a reason for hiding this comment

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

I think this LGTM ! I agree with your strategy @tsalo to see how it works in production, and tweak from there (e.g., if we need to modify who can cut the release).

Thanks !!

@tsalo tsalo requested a review from handwerkerd August 8, 2020 19:28
Copy link
Collaborator

@jbteves jbteves left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @tsalo !

@tsalo
Copy link
Member Author

tsalo commented Aug 21, 2020

Thanks all! Merging now.

@tsalo tsalo merged commit 75bdda8 into ME-ICA:master Aug 21, 2020
@tsalo tsalo deleted the maint/autodeploy-pypi branch August 21, 2020 14:59
@tsalo tsalo mentioned this pull request Aug 21, 2020
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