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

fix: Correct reference check in GitHub Actions publish to TestPyPI step #640

Merged
merged 7 commits into from
Nov 15, 2019

Conversation

kratsg
Copy link
Contributor

@kratsg kratsg commented Nov 15, 2019

Description

This adds in changes to #639. GHA triggers pushes on PRs as well... so you can't tell if a push event comes from a PR or not. Also need to fix logic of whether to use scm or not depending on if we're tagged or not.

Checklist Before Requesting Reviewer

  • Tests are passing
  • "WIP" removed from the title of the pull request
  • Selected an Assignee for the PR to be responsible for the log summary

Before Merging

For the PR Assignees:

  • Summarize commit messages into a comprehensive review of the PR
* More fixes to workflow in #639, only push to testpypi on pushes on master
* Only use scm if the current commit is not tagged
* Protect forks from publishing

@codecov
Copy link

codecov bot commented Nov 15, 2019

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #640   +/-   ##
=======================================
  Coverage   94.93%   94.93%           
=======================================
  Files          42       42           
  Lines        2547     2547           
  Branches      345      345           
=======================================
  Hits         2418     2418           
  Misses         85       85           
  Partials       44       44
Flag Coverage Δ
#unittests 94.93% <ø> (ø) ⬆️

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 6f50690...2df6ea8. Read the comment docs.

@matthewfeickert matthewfeickert added CI CI systems, GitHub Actions fix A bug fix labels Nov 15, 2019
@matthewfeickert
Copy link
Member

matthewfeickert commented Nov 15, 2019

For posterity, to document the logic that is being used here that @kratsg had to explain to me as I missed it: Given that

GHA triggers pushes on PRs as well... so you can't tell if a push event comes from a PR or not.

this means that

  1. First, you have to check if the commit is tagged (and so meant for PyPI). If it is, then skip using scm. If it is not
if getenv('IS_COMMIT_TAGGED') == 'false'

then use scm and this commit will get pushed up to TestPyPI. Then build the release with whatever release number scheme you are using. This affects the release number of what is built and so needs to happen first.

  1. As every PR will trigger a push event on master then you need to check to make sure that the reference for the push event is actually coming from master
if: github.event_name == 'push' && github.ref == 'refs/heads/master'
  1. If the commit is tagged, then push to PyPI. You shouldn't try to check here for the branch as the only information that github.ref can give is the tag, not which branch the tag was coming from.
if: github.event_name == 'push' && startsWith(github.ref, 'refs/tags')

This largely comes from the work @kratsg did on stare. As he points out though, one weakness it that any branch that has a tag pushed to it can trigger a deploy to PyPI. This should ideally be defended against in the future.

@matthewfeickert matthewfeickert changed the title fix: more workflow fixes fix: Correct reference check in GitHub Actions publish to TestPyPI step Nov 15, 2019
Co-Authored-By: Matthew Feickert <matthew.feickert@cern.ch>
Copy link
Member

@matthewfeickert matthewfeickert left a comment

Choose a reason for hiding this comment

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

Thank you @kratsg for the fix. Also, thank you for doing all the work already in stare.

@matthewfeickert matthewfeickert merged commit 528b009 into master Nov 15, 2019
@matthewfeickert matthewfeickert deleted the fix/buildSystem branch November 15, 2019 05:21
@kratsg
Copy link
Contributor Author

kratsg commented Nov 15, 2019

We're safe from malicious PRs: maxheld83/ghactions#262 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI CI systems, GitHub Actions fix A bug fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants