-
Notifications
You must be signed in to change notification settings - Fork 667
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
Migrate publishing to PyPI from Travis CI to GHA #840
Conversation
(decided that dropping travis can be done separately from moving the publish procedure) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general it looks ok but please:
- remove emoji
- assure that it does not run if linters are failing (you used condition only on test-matrix, which does not include linters).
This is out of scope, plus I don't think that linters should block the release. That's why they are in a separate job. |
When I say linters, I mean Regarding emoji, lets not impose our personal preferences and see what other have to say about it. I am aware of lots of projects that forbid use of emoji in commit messages or code (openstack is one of them). |
Exactly, it's not worth blocking a good change. You didn't suggest a better solution anyway and this one already conforms my requirements to that name.
They will still show up on PRs and you're not going to be tagging PR branches, right? What if something changes in the env and a perfectly valid and green, at the moment of merging, commit publishing will ruin the publish just because some linter changed or something silly broke? I've put them separately for a reason and I exercise this set up in many places and it works fine. The only reason for linters to be run early in Travis is to fail early, preventing the tests from running, not publishing. That's because it has way more limited resources that we wanted to consume moderately. |
FTR build-dists and metadata-validation are in the deploy job already. A and |
9072def
to
99fb183
Compare
I don't like doing this, though I'm just going to pull rank here. |
Thanks @gundalow, adding an approval should do. Whoever does this, please feel free to merge it yourself since I'm mostly afk this weekend + PTO on Mon. |
I can't review this from a technical level, so I'll leave that in the good hands of @awcrosby @ssbarnea and @albinvass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No offense, but I am keeping my downvote because previous mistakes should not be used as an excuse to make the situation worse (more emoji).
Obviously that others are welcomed to have different opinions and I would remove my vote if I see I am alone in my views.
There is an already existing review to remove the existing emoji at #845 -- luckily since we already removed the azure pipelines we have only one more file to fix.
No description provided.