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

Add Github Actions and derive version from git tags #471

Merged
merged 3 commits into from
Aug 10, 2021

Conversation

lpetre
Copy link
Contributor

@lpetre lpetre commented Mar 31, 2021

Summary

Two changes here:

  • I've ported the circleci config to github actions. I propose this as a replacement to circleci
  • I've added setuptools_scm and configured it to generate libcst\_version.py

To improve our workflow around libcst releases, I'd like to propose that we publish pre-release versions of the package.

More context about publishing here: https://packaging.python.org/guides/publishing-package-distribution-releases-using-github-actions-ci-cd-workflows/#publishing-the-distribution-to-pypi-and-testpypi

And pre-release versions here: https://packaging.python.org/guides/distributing-packages-using-setuptools/#pre-release-versioning

Relating that to this change:

  • setuptools_scm makes the pre-release versioning trivial
  • We can add ~10 lines of github action yaml to handle the publishing to PyPi

TODOs:

  • I will follow up with a commit that handles the pypi publishing
  • I will delete the circleci files
  • If this proposal is acceptable, we would need to configure some secrets in Github (ie codecov and pypi tokens)
  • Add testing on windows-latest. I discovered that the test suite doesn't pass on windows, which I will fix in another PR.

Test Plan

See https://github.com/lpetre/LibCST/actions/runs/704265869

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Mar 31, 2021
Copy link
Member

@zsol zsol left a comment

Choose a reason for hiding this comment

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

I like this direction, but let's make sure the CI setup is healthy before changing the release process.

tox.ini Show resolved Hide resolved
Comment on lines +198 to +199
except ImportError:
LIBCST_VERSION = "unknown"
Copy link
Member

Choose a reason for hiding this comment

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

when can this condition trigger?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think @thatch's comment is meant to be a reply here.

#471 (comment)

If someone adds it to pythonpath without using setup.py develop or similar.

@codecov-commenter
Copy link

codecov-commenter commented Aug 9, 2021

Codecov Report

Merging #471 (4798164) into master (a1282f2) will decrease coverage by 0.00%.
The diff coverage is 50.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #471      +/-   ##
==========================================
- Coverage   94.70%   94.70%   -0.01%     
==========================================
  Files         236      235       -1     
  Lines       23084    23077       -7     
==========================================
- Hits        21861    21854       -7     
  Misses       1223     1223              
Impacted Files Coverage Δ
libcst/tool.py 33.33% <ø> (ø)
setup.py 100.00% <ø> (+13.33%) ⬆️
libcst/__init__.py 91.30% <50.00%> (-8.70%) ⬇️

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 a1282f2...4798164. Read the comment docs.

@thatch
Copy link
Contributor

thatch commented Aug 9, 2021 via email

@lpetre lpetre requested a review from zsol August 9, 2021 20:57
@lpetre
Copy link
Contributor Author

lpetre commented Aug 9, 2021

Note: I have a follow up PR w/ windows testing (and a windows fix).

Comment on lines +2 to +8
envlist = py36, py37, py38, lint, docs

[gh-actions]
python =
3.6: py36
3.7: py37
3.8: py38
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not 3.9?

Move to GH actions LGTM.

Copy link
Member

Choose a reason for hiding this comment

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

Currently circleci doesn't cover 3.9. I'd prefer adding 3.9 in a separate PR, this is already complex enough

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm happy to follow up w/ 3.9 support in a separate PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

@zsol zsol left a comment

Choose a reason for hiding this comment

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

LGTM. if you resolve the conflict I can merge

@lpetre
Copy link
Contributor Author

lpetre commented Aug 10, 2021

LGTM. if you resolve the conflict I can merge

Resolved!

@zsol zsol changed the title [RFC] Adding Github Actions and derive version from git tags Add Github Actions and derive version from git tags Aug 10, 2021
@zsol zsol merged commit 1c3a27b into Instagram:master Aug 10, 2021
@lpetre lpetre deleted the github_actions branch August 10, 2021 18:29
lpetre added a commit to lpetre/LibCST that referenced this pull request Aug 10, 2021
* Use setuptools-scm to derive the current version from git metadata

* Add Github Action equivalent to the current circleci tasks

* Run pyre integration test in GH action / tox
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants