Skip to content

Add twine check to GitHub Actions#59291

Merged
Ch3LL merged 10 commits intosaltstack:masterfrom
ScriptAutomate:twine-check
Feb 5, 2021
Merged

Add twine check to GitHub Actions#59291
Ch3LL merged 10 commits intosaltstack:masterfrom
ScriptAutomate:twine-check

Conversation

@ScriptAutomate
Copy link
Contributor

What does this PR do?

Runs twine check against PRs to ensure nothing is breaking.

What issues does this PR fix or reference?

Adds automated check suggested in #58727

Previous Behavior

No twine check was being done, so changes to something like README.rst could prevent twine from later publishing to PyPi further down the pipeline for a release.

New Behavior

twine check is now done against each PR, to ensure no PyPi publishing issues related to twine check happen at release.

Commits signed with GPG?

Yes

Additional Notes

This PR is dependent should only be merged once #59289 is merged, as that PR fixes the current problem with twine check failing against the README.rst file.

@ScriptAutomate ScriptAutomate changed the title Add twin check to GitHub Actions Add twine check to GitHub Actions Jan 13, 2021
s0undt3ch
s0undt3ch previously approved these changes Jan 13, 2021
@bryceml
Copy link
Contributor

bryceml commented Jan 14, 2021

you probably want the depth on the git checkout to include at least the last tag since salt creates different file names and version based on the last tag it finds.

It would also be good to cache the pip stuff in case pypi has issues and to lighten the load on pypi in general, maybe something like this: espressif/esptool@c325e36

it might be better to cache an entire virtual environment so pypi doesn't have to be up at all for it to work.

@Ch3LL
Copy link
Contributor

Ch3LL commented Feb 4, 2021

@ScriptAutomate where you able to see @bryceml 's comments? Would be great to get this one in for Aluminium.

@Ch3LL Ch3LL added the Aluminium Release Post Mg and Pre Si label Feb 4, 2021
@ScriptAutomate
Copy link
Contributor Author

ScriptAutomate commented Feb 4, 2021

you probably want the depth on the git checkout to include at least the last tag since salt creates different file names and version based on the last tag it finds.

It would also be good to cache the pip stuff in case pypi has issues and to lighten the load on pypi in general, maybe something like this: espressif/esptool@c325e36

it might be better to cache an entire virtual environment so pypi doesn't have to be up at all for it to work.

Great idea, @bryceml. That's for GitLab CI, so I poked around for how people approach this via GitHub Actions, and found a good write-up by Evan Pete Walsh / @epwalsh over at AI2:

Tested out, and seems to work well! I also did the following:

  • Pinned to ubuntu-20.04 since GitHub Actions is planning to migrate ubuntu-latest over to it. This helped confirm it won't break on that migration.
  • The cache will change if the version of Python updates, if the workflow file itself updates for twine check, or if README.rst updates.
    • twine is pinned to 3.3.0 in the workflow, but can be updated later (unless we want this pinned like >=3.3.0 instead? Cache will update anytime we choose to modify :)
  • Didn't pass any args to checkout, since twine check seems focused on README.rst specifically

Also, the PR blocking this PR has been merged, and Twine checks pass against master! So, this PR is officially ready for review.

@ScriptAutomate ScriptAutomate marked this pull request as ready for review February 4, 2021 22:15
@ScriptAutomate ScriptAutomate requested a review from a team as a code owner February 4, 2021 22:15
@ScriptAutomate ScriptAutomate requested review from twangboy and removed request for a team February 4, 2021 22:15
@Ch3LL Ch3LL merged commit 45c0352 into saltstack:master Feb 5, 2021
@ScriptAutomate ScriptAutomate deleted the twine-check branch February 6, 2021 20:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Aluminium Release Post Mg and Pre Si

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants