-
Notifications
You must be signed in to change notification settings - Fork 36
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
🎨 Make the GHA log clean and colorized #66
Conversation
This patch sets up root-level environment variables shared by all the workflow jobs. They include: * Disabling undesired `pip`'s warnings/suggestions * Requesting the executed apps color their output unconditionally * Letting `tox` pass those requests to underlying/wrapped programs
d44b7f1
to
e22ae08
Compare
@jaraco pytest output is colored here, in the test run: https://github.com/jaraco/skeleton/actions/runs/3166345584/jobs/5155999263#step:5:32. pip's output is not yet colored but it will be at some point: pypa/pip#10909 (comment). By the way, have you considered creating a |
.github/workflows/main.yml
Outdated
PIP_NO_WARN_SCRIPT_LOCATION: 1 | ||
PY_COLORS: 1 # Recognized by the `py` package, dependency of `pytest` | ||
TOX_PARALLEL_NO_SPINNER: 1 | ||
TOX_TESTENV_PASSENV: >- # Make tox-wrapped tools see color requests |
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.
I imagine this env var is going to conflict or be overridden in some projects that use it for other purposes. I wonder if tox should support these by default. Have you explored that with the project?
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.
My understanding is that these could actually go into tox.ini
.
.github/workflows/main.yml
Outdated
PIP_NO_PYTHON_VERSION_WARNING: 1 | ||
PIP_NO_WARN_SCRIPT_LOCATION: 1 | ||
PY_COLORS: 1 # Recognized by the `py` package, dependency of `pytest` | ||
TOX_PARALLEL_NO_SPINNER: 1 |
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.
Tox should probably support this by default. If it's known to be noisy in GHA, it should detect GHA and disable it automatically. Can you file a request please?
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.
IIRC it was rather controversial when it was first implemented in tox. Some toggles are only changeable via CLI or env vars, but not though config settings. The maintainers weren't open to improving this.
This patch sets up root-level environment variables shared by all the workflow jobs. They include: * Disabling undesired `pip`'s warnings/suggestions * Requesting the executed apps color their output unconditionally * Letting `tox` pass those requests to underlying/wrapped programs
PIP_NO_WARN_SCRIPT_LOCATION: true | ||
|
||
# Disable the spinner, noise in GHA; TODO(webknjaz): Fix this upstream | ||
TOX_PARALLEL_NO_SPINNER: true |
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.
Seems true
isn't allowed (mea culpa). I merged it, but amended in ad6091d.
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.
That's weird. I've been using it like this for a while, and it did work...
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.
Oh, wait, you changed it from 1
. That explains it. Some apps expect 1
specifically.
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.
That's annoying and slightly horrible. I try to reserve literal numbers for quantities. The number is written in ascii, ported to an integer in yaml, written to the environment as a string, then loaded from the environment as a string and cast to an int before being cast to a bool (https://github.com/python/mypy/blob/0cab54432a1df91a9b71637b773bcbb9772a6b59/mypy/util.py#L523). Moreover, mypy's interpretation of FORCE_COLOR is a mismatch from the published expectation in other environments (where any non-empty value means true, even 0
).
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.
Given all this complication, I'm inclined to back out this merge. The landscape is complicated enough that it probably deserves its own CI plugin. How do you feel about developing a plugin/action that would set these environment variables (and manage the complexity) so it doesn't have to happen here?
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.
I've force-pushed b241226 back to main, backing out this change. I'd very much like to see it, but it's clearly fraught with micro-challenges.
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.
I did think about having an action. That would be pretty easy. But decided that it's not what I want because this action would have to be copied into every job and the original reason I moved these to the root level of the workflow was to reduce duplication.
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.
Oh, good point. It would be annoying to have an action scattered across any number of jobs. That's an interesting tradeoff. Do you happen to know if Github has plans to support a "global" or "workspace" level action such that it could run at the beginning of all jobs? I searched and found people wanting such a thing, but they're all satisfied with environment variables or defaults.
Would you be willing to file a request with Github explaining this motivation?
In the meantime, I'd like to adopt your changes again, this time keeping the 1
s (holding my nose for the smell) but retaining the comments. I'll plan to do that soon, maybe tomorrow.
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.
Dunno, knowing how GHA workflows work and how they are isolated, I find it hard to imagine how such a feature could work. Also, I don't have a lot of energy this year to track so many efforts. So no, I won't be requesting this a feature atm.
Meanwhile, I've returned to working/experimenting on my old idea of autogeneration of a GHA job matrix for tox. Here's a PoC demo if you haven't seen it: https://github.com/webknjaz/tox-gha-test/actions/runs/3199728755.
I'm willing to bet that having reusable workflows could significantly reduce the number of places where this would have to be copy-pasted.
I'm moving this summary about
|
This patch sets up root-level environment variables shared by all the workflow jobs. They include:
pip
's warnings/suggestionstox
pass those requests to underlying/wrapped programs