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.

Sign up for GitHub

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

Jump to bottom

🎨 Make the GHA log clean and colorized #66

Merged
merged 3 commits into from
Oct 5, 2022
Merged

🎨 Make the GHA log clean and colorized #66

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e22ae08
🎨 Make the GHA log is clean and colorized
webknjaz Oct 1, 2022
f7f0b03
Reformat without end of line comments. Group into sections.
jaraco Oct 5, 2022
7e264ac
Prefer boolean value for boolean vars. Reserve numeric literals for q…
jaraco Oct 5, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions .github/workflows/main.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,32 @@ name: tests

on: [push, pull_request]

env:
# Color support (jaraco/skeleton#66)
# Request colored output from CLI tools supporting it
FORCE_COLOR: true
# MyPy's color enforcement
MYPY_FORCE_COLOR: true
# Recognized by the `py` package, dependency of `pytest`
PY_COLORS: true
# Make tox-wrapped tools see color requests
TOX_TESTENV_PASSENV: >-
FORCE_COLOR
MYPY_FORCE_COLOR
NO_COLOR
PY_COLORS
PYTEST_THEME
PYTEST_THEME_MODE

# Suppress noisy pip warnings
PIP_DISABLE_PIP_VERSION_CHECK: true
PIP_NO_PYTHON_VERSION_WARNING: true
PIP_NO_WARN_SCRIPT_LOCATION: true

# Disable the spinner, noise in GHA; TODO(webknjaz): Fix this upstream
TOX_PARALLEL_NO_SPINNER: true
Copy link
Owner

@jaraco jaraco Oct 5, 2022 •
edited
Loading

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.

Copy link
Contributor Author

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

Copy link
Contributor Author

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.

Copy link
Owner

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

Copy link
Owner

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?

Copy link
Owner

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.

Copy link
Contributor Author

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.

Copy link
Owner

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 1s (holding my nose for the smell) but retaining the comments. I'll plan to do that soon, maybe tomorrow.

Copy link
Contributor Author

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.



jobs:
test:
strategy:
Expand Down