Skip to content

Multi Platform CI Pipeline with GitHub Actions#651

Merged
sigmavirus24 merged 12 commits intopypa:masterfrom
callmecampos:multi-platform-ci
Jun 10, 2020
Merged

Multi Platform CI Pipeline with GitHub Actions#651
sigmavirus24 merged 12 commits intopypa:masterfrom
callmecampos:multi-platform-ci

Conversation

@callmecampos
Copy link
Copy Markdown
Contributor

@callmecampos callmecampos commented Jun 4, 2020 •
edited
Loading

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
Copy Markdown
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
Copy Markdown
Member

sigmavirus24 commented Jun 6, 2020

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

@di
Copy link
Copy Markdown
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
Copy Markdown
Member

sigmavirus24 commented Jun 6, 2020

@callmecampos can you resolve the merge conflicts here?

@callmecampos
Copy link
Copy Markdown
Contributor Author

callmecampos commented Jun 6, 2020

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

bhrutledge
bhrutledge reviewed Jun 6, 2020
View reviewed changes
Copy link
Copy Markdown
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.

Comment thread tox.ini Outdated
Comment thread tests/test_wheel.py Outdated
Comment thread tests/test_wheel.py Outdated
Comment thread tests/test_wheel.py
Comment thread tests/test_integration.py
@callmecampos
Copy link
Copy Markdown
Contributor Author

callmecampos commented Jun 8, 2020

@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.

@bhrutledge bhrutledge mentioned this pull request Jun 9, 2020
Closed
bhrutledge
bhrutledge requested changes Jun 9, 2020
View reviewed changes
Copy link
Copy Markdown
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.

Comment thread .github/workflows/main.yml Outdated
@bhrutledge
Copy link
Copy Markdown
Contributor

bhrutledge commented Jun 9, 2020

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
Copy Markdown
Contributor Author

callmecampos commented Jun 9, 2020 •
edited
Loading

@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.

di
di reviewed Jun 9, 2020
View reviewed changes
Comment thread .github/workflows/main.yml Outdated
@di di requested a review from bhrutledge June 9, 2020 23:50
bhrutledge
bhrutledge approved these changes Jun 10, 2020
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
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
Closed
7 tasks
@di
Copy link
Copy Markdown
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

Reviewers

@di di di approved these changes

@bhrutledge bhrutledge bhrutledge approved these changes

@sigmavirus24 sigmavirus24 Awaiting requested review from sigmavirus24

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@callmecampos @di @sigmavirus24 @bhrutledge