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 CI workflow for GitHub Actions #1012

Merged
merged 6 commits into from May 15, 2020
Merged

Conversation

atugushev
Copy link
Member

No description provided.

@atugushev atugushev force-pushed the gh-actions-ci branch 2 times, most recently from 0ae3a35 to cc800ac Compare November 27, 2019 19:21
@codecov
Copy link

codecov bot commented Nov 27, 2019

Codecov Report

Merging #1012 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1012   +/-   ##
=======================================
  Coverage   99.47%   99.47%           
=======================================
  Files          37       37           
  Lines        2675     2680    +5     
  Branches      322      322           
=======================================
+ Hits         2661     2666    +5     
  Misses          8        8           
  Partials        6        6           
Impacted Files Coverage Δ
tests/conftest.py 100.00% <100.00%> (ø)
tests/utils.py 100.00% <100.00%> (ø)

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 ce3da73...1a45ee3. Read the comment docs.

@atugushev atugushev force-pushed the gh-actions-ci branch 9 times, most recently from f75bd55 to 56de8bf Compare November 28, 2019 10:05
@atugushev
Copy link
Member Author

@atugushev atugushev force-pushed the gh-actions-ci branch 12 times, most recently from 0c5958d to b743655 Compare February 1, 2020 03:07
@atugushev atugushev force-pushed the gh-actions-ci branch 4 times, most recently from 7309dcd to 294e31d Compare March 5, 2020 09:22
@atugushev atugushev closed this Mar 6, 2020
@atugushev
Copy link
Member Author

atugushev commented May 3, 2020

Hey @webknjaz. Thank you so much for such huge feedback! Much appreciated! ❤️ I've resolved all the discussions (I hope, feel free to unresolve otherwise) and I'm ready for another round of review! 🚀

Copy link
Member

@webknjaz webknjaz left a comment

Choose a reason for hiding this comment

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

LGTM overall

Copy link
Member

@codingjoe codingjoe left a comment

Choose a reason for hiding this comment

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

Looks good. I am not sure I need tox in a GitHub actions world, but that is certainly a question of preference.

What the only point I'd stress on, would be to merge both workflows. Both have the same running condition and only one job. IMHO it should be one workflow with two or more jobs, preferable a dependent one. A workflow can be as big as this: https://github.com/codingjoe/django/blob/30962/flake8/.github/workflows/ci.yml

.github/workflows/ci.yml Show resolved Hide resolved
.github/workflows/qa.yml Show resolved Hide resolved
@webknjaz
Copy link
Member

webknjaz commented May 4, 2020

Looks good. I am not sure I need tox in a GitHub actions world, but that is certainly a question of preference.

It's needed with any CI because it works like a glue between different platforms, plus it allows running tests locally exactly the same way. It's a well-established practice to use it like this.

@webknjaz
Copy link
Member

webknjaz commented May 4, 2020

@atugushev maybe on the next iteration we could make the CI actually test the dists too, not just source. And add macOS into the matrix. But right now it seems ready to go. Done is better than perfect.

@atugushev
Copy link
Member Author

atugushev commented May 4, 2020

@webknjaz

maybe on the next iteration we could make the CI actually test the dists too, not just source. And add macOS into the matrix. But right now it seems ready to go. Done is better than perfect.

What do you mean by "test the dists"?

@webknjaz
Copy link
Member

webknjaz commented May 4, 2020

What do you mean by "test the dists"?

When you run tox, it works with the source in the project dir and even creates some temporary dists. But they are not what you're going to publish to PyPI hence you test not exactly the same as users will get. Also, you may be testing not what's installed for various reasons.
Better way would be to divide this process into stages:

  (1) build dists -> (2) test dists from [1] -> (3) upload *exactly* those dists from [1] that have been tested

@atugushev atugushev mentioned this pull request May 4, 2020
@atugushev
Copy link
Member Author

@webknjaz

Good suggestion! There is an issue #1134 to track.

Co-Authored-By: Sviatoslav Sydorenko <wk.cvs.github@sydorenko.org.ua>
@atugushev
Copy link
Member Author

@webknjaz

I've added MacOS to the matrix.

@atugushev
Copy link
Member Author

image

pypy test jobs are so slow 😞 By the way pip doesn't run tests on pypy, see pypa/pip#7279 (comment). It's tempting to turn them off...

@webknjaz
Copy link
Member

webknjaz commented May 4, 2020

You could add a condition to only run them on cron and push, for example

@@ -1,28 +1,73 @@
name: cron
name: Cron
Copy link
Member

Choose a reason for hiding this comment

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

Oh, I didn't think you'd copy all the workflow into another file. I meant we could inject a small if-clause on the job level to filter out things.

Copy link
Member Author

Choose a reason for hiding this comment

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

Could you post a suggestion?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm okay with the copy approach for a while. As soon as GitHub provides an opportunity to make matrix conditionals at a job level we'll refactor it.

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Wow 😄


jobs:
test:
name: ${{ matrix.os }} / ${{ matrix.python-version }} / ${{ matrix.pip-version }}
Copy link
Member

@webknjaz webknjaz May 4, 2020

Choose a reason for hiding this comment

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

Suggested change
name: ${{ matrix.os }} / ${{ matrix.python-version }} / ${{ matrix.pip-version }}
name: ${{ matrix.os }} / ${{ matrix.python-version }} / ${{ matrix.pip-version }}
if: >-
!startsWith(matrix.python-version, 'pypy') ||
github.event_name != 'pull_request'

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm... In that case test job is not run.

Copy link
Member

Choose a reason for hiding this comment

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

Why? It should only exclude matching jobs.

Copy link
Member

Choose a reason for hiding this comment

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

I may have made a mistake in the condition, though.

Copy link
Member

Choose a reason for hiding this comment

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

No, it looks correct

Copy link
Member

@webknjaz webknjaz May 4, 2020

Choose a reason for hiding this comment

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

That's because you have to apply my suggestion exactly like it's submitted. But you added a leading ! there w/o quoting. This works w/o quoting only if you wrap the whole value with quotes or use a multiline form as I originally suggested.

Copy link
Member

Choose a reason for hiding this comment

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

It's not a context access problem but a YAML parsing one.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I see. Maybe you're right but that post is old, have you checked if it works with the proper syntax?

Copy link
Member Author

Choose a reason for hiding this comment

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

Screenshot 2020-05-05 at 04 35 23

Nope, doesn't work.

Copy link
Member

Choose a reason for hiding this comment

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

:(

@webknjaz webknjaz requested a review from codingjoe May 4, 2020 20:56
Copy link
Contributor

@AndydeCleyre AndydeCleyre left a comment

Choose a reason for hiding this comment

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

You and @webknjaz are thoroughly ironing this out. 🙌 👍

.github/workflows/ci.yml Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
@webknjaz
Copy link
Member

webknjaz commented May 6, 2020

@codingjoe do you still want to block this?

@atugushev
Copy link
Member Author

Seems like it's ready to go! 🚀

@atugushev atugushev merged commit b1d7369 into jazzband:master May 15, 2020
@atugushev atugushev deleted the gh-actions-ci branch May 15, 2020 15:14
@atugushev
Copy link
Member Author

I'm so happy we've finally done this. Huge thanks to the reviewers! ❤️

atugushev added a commit to atugushev/pre-commit.github.io that referenced this pull request Dec 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci Related to continuous integration tasks maintenance Related to maintenance processes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants