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

Multi Platform CI Pipeline with GitHub Actions #651

Merged
merged 12 commits into from
Jun 10, 2020

Conversation

callmecampos
Copy link
Contributor

@callmecampos callmecampos commented Jun 4, 2020

Per discussion in #650, this PR leverages GitHub Actions to test twine against Ubuntu, macOS, and Windows.

This involved some changes to the twine.wheel unit tests, specifically surrounding the assumption of Unix/Posix environments for path representations. To fix this, we now use pathlib in the wheel unit tests to represent paths, since when initializing a pathlib.Path object, pathlib automatically returns a PosixPath or a WindowsPath dependent on the operating system at runtime.

Additionally, integration tests test_devpi_upload and test_pypi_server_upload are expected to fail on Windows and thus skipped since they're using pytest-services' watcher-getter fixture, which doesn't support Windows according to pytestservices:#22.

There has also been discussion in #650 about replacing Travis entirely with GitHub Actions, which is reasonable enough, though .travis.yml also includes a code coverage step (codecov --env TRAVIS_OS_NAME, TOX_ENV) which I don't believe we currently have with GitHub Actions. Talking with @di, we could move to using a different coverage tool like https://pypi.org/project/coverage/. This can be done in this PR or a later one which migrates away from Travis completely towards GitHub Actions.

Update 6/5/20: Added code coverage to the config file.

@di
Copy link
Member

di commented Jun 4, 2020

Also, since @callmecampos is not a maintainer, the action won't run on this PR, but you can see it running on his branch here: https://github.com/callmecampos/twine/actions?query=branch%3Amulti-platform-ci

@callmecampos callmecampos force-pushed the multi-platform-ci branch 13 times, most recently from e68a540 to f196e89 Compare June 5, 2020 21:50
@sigmavirus24
Copy link
Member

What would the steps be after merging this, @di?

@di
Copy link
Member

di commented Jun 6, 2020

Aside from removing the Travis config in a separate PR, nothing. Once merged it will run on all PRs and pushes.

@sigmavirus24
Copy link
Member

@callmecampos can you resolve the merge conflicts here?

@callmecampos
Copy link
Contributor Author

@sigmavirus24 merge conflicts resolved and new CI checks passed here: https://github.com/callmecampos/twine/actions/runs/127062822

Copy link
Contributor

@bhrutledge bhrutledge left a comment

Choose a reason for hiding this comment

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

Apologies for the late review here. I noticed a few things, but they don't need to hold up a merge; happy to address them later.

tox.ini Outdated Show resolved Hide resolved
tests/test_wheel.py Outdated Show resolved Hide resolved
tests/test_wheel.py Outdated Show resolved Hide resolved
tests/test_wheel.py Show resolved Hide resolved
tests/test_integration.py Show resolved Hide resolved
@callmecampos
Copy link
Contributor Author

@bhrutledge I made the changes you suggested, thanks for reviewing!

Also, according to codecov/codecov-action#37 there have been issues with codecov uploads timing out with GitHub Actions, so instead of uploading Coverage.py's XML report using python -m coverage xml, I'm just running python -m coverage report which directly prints out the report per @di's suggestion. See https://github.com/callmecampos/twine/actions/runs/128994913 for the most recent passing GitHub Actions checks.

Copy link
Contributor

@bhrutledge bhrutledge left a comment

Choose a reason for hiding this comment

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

Thanks! One small change to avoid redundancy.

That's a bummer re: Codecov. I opened #658 to consider replacing it.

Also, looking at the commit history, I think this should be squashed, either using the "Squash & Merge" button when it's ready, or locally via git rebase -i.

.github/workflows/main.yml Outdated Show resolved Hide resolved
@bhrutledge
Copy link
Contributor

Following up on this:

there have been issues with codecov uploads timing out with GitHub Actions

@callmecampos, have you seen timeouts in practice? From a GitHub search, it looks this action is widely used, including by some high-profile Python projects:

With that list, I guess I'd be surprised if it's not safe to use.

@callmecampos
Copy link
Contributor Author

callmecampos commented Jun 9, 2020

@bhrutledge Yeah, I was seeing it sporadically yesterday. If you go to this run here, and click on the failing job (Upload coverage to Codecov) you'll see an example of it failing because of timing out.

.github/workflows/main.yml Outdated Show resolved Hide resolved
@di di requested a review from bhrutledge June 9, 2020 23:50
Copy link
Contributor

@bhrutledge bhrutledge left a comment

Choose a reason for hiding this comment

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

@callmecampos Thanks for the link. Now that you mention it, I have seen those errors before; I usually just restart the job when that happens.

As long as we still have Travis, I think this Actions setup is great. When we drop Travis, we'll need to add types, docs, and release stages. It might also be worth revisiting Codecov.

@di
Copy link
Member

di commented Jun 10, 2020

I've added a checklist which includes all of those to #650.

@sigmavirus24 sigmavirus24 merged commit f922eeb into pypa:master Jun 10, 2020
@di di mentioned this pull request Jun 10, 2020
7 tasks
@di
Copy link
Member

di commented Jun 10, 2020

You can now see the successful run here: https://github.com/pypa/twine/actions/runs/131530548

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