chore(build/ci): switch to uv and ruff #5777
Conversation
dosubot
bot
added
size:XL
the-wondersmith
force-pushed
the
dev/switch-to-uv-and-ruff
branch
from
1989507
to
b8f9416
Compare
dosubot
bot
added
size:XXL
the-wondersmith
force-pushed
the
dev/switch-to-uv-and-ruff
branch
from
537dbee
to
ac07a87
Compare
the-wondersmith
force-pushed
the
dev/switch-to-uv-and-ruff
branch
from
ff56e0b
to
cc29734
Compare
the-wondersmith
force-pushed
the
dev/switch-to-uv-and-ruff
branch
from
e8b5fa9
to
a530ff7
Compare
dosubot
bot
added
size:XL
There was a problem hiding this comment.
Man, this is a huge PR. 😂 Approving with some comments that can be addressed in a future PR. The ones I feel most strongly about are that we shouldn't really have any #noqa: E402 comments (we should just be able to move the imports up above the executable code) and that I'd really prefer == when comparing specifically with True and False...
Thanks much!! This must've been a huge, huge, massive pain.
| def __init__(self, local_config_path: str) -> None: | ||
| self.local_path = local_config_path | ||
| self.notices: List[Dict[str, str]] = [] | ||
| self.local_path, self.notices = local_config_path, [] |
There was a problem hiding this comment.
I think these are more readable on two lines. They're not related to each other, and vertical space is plentiful these days.
|
|
||
| def reset(self): | ||
| local_notices: List[Dict[str, str]] = [] | ||
| local_data = "" | ||
| local_data, local_notices = "", [] |
| self, cmd: str, arg: Optional[Union[str, Tuple[str, Optional[IR]], Tuple[str, str]]] | ||
| self, | ||
| cmd: str, | ||
| arg: Optional[Union[str, Tuple[str, Optional[IR]], Tuple[str, str]]], |
There was a problem hiding this comment.
This makes me wonder why I didn't give this type a name back when. 😂
| % ( | ||
| config_type, | ||
| snapshot, | ||
| service_count, | ||
| listener_count, | ||
| group_count, | ||
| cluster_count, | ||
| ) |
There was a problem hiding this comment.
(Why does ruff like doing this? 🤦♂️)
There was a problem hiding this comment.
Because ruff preserves behavior from black, and therefore because of the trailing comma rule in black [ref], at least I assume?
| @@ -143,7 +149,7 @@ def _test_errorresponse(yaml, expectations, expect_fail=False): | |||
| % (expected_body_format_override, expected_filter) | |||
| ) | |||
| print("checking m: ", m) | |||
| actual_filter = m["filter"] | |||
| # actual_filter = m["filter"] | |||
There was a problem hiding this comment.
Another one to just delete, I think.
| # Import all the real tests from other files, to make it easier to pick and choose during development. | ||
|
|
There was a problem hiding this comment.
The comment should go if all the imports have gone.
IIRC the point of all this stuff had to do with the way pytest found tests -- I assume that's different now...
| @@ -121,7 +124,7 @@ def check(self): | |||
| elif "X-Envoy-Overloaded" in result.headers: | |||
| pending_overloaded += 1 | |||
|
|
|||
| failed = False | |||
| # failed = False | |||
There was a problem hiding this comment.
Thinking this can just be deleted.
| @@ -122,7 +128,7 @@ def check(self): | |||
| valid += 1 | |||
| elif self.results[i].status == 500: | |||
| errors += 1 | |||
| msg = "Errors: {}, Valid: {}".format(errors, valid) | |||
| # msg = "Errors: {}, Valid: {}".format(errors, valid) | |||
There was a problem hiding this comment.
Should just delete this. Originally I think it was used as the assertion string (cf the ActiveHealthCheckTest function earlier in this file) but it's a terrible string for that anyway.
| env: | ||
| DEBIAN_FRONTEND: noninteractive | ||
| run: | | ||
| python -m pip install -U awscli packaging | ||
| uv venv | ||
| uv --no-config pip install -U awscli |
There was a problem hiding this comment.
I think awscli is in the dev dependencies now, no?
(I also expect we'll likely get rid of it – I think it's only ALabs-specific stuff.)
There was a problem hiding this comment.
I did move everything else that used to be installed in that step into the dev deps, but nothing in the Python packages explicitly used anything from awscli, so I figured maybe it was a CI-specific thing and just left it.
We can absolutely sunset it / remove it entirely though, if it's an ALabs-exclusive
I'll get to work on them posthaste 😅
Agreed. For historical purposes though - I left the imports "out of order" and added the
Ummm.... that might be a sticking point? There actually is a meaningful difference between
My pleasure! It wasn't that big a pain - labors of |
the-wondersmith
merged commit d541135
into
emissary-ingress:dev/v4
PR completes the "migration" to fully using
uvandrufffor python environment management and linting/formatting (respectively).There are numerous changes to the
.pyfiles, (and they should be eyeballed at the very least), but they're all just linting fixes to get the python side of things "up to snuff" with ruff's configured lints.The important parts here that actually require significant scrutiny are the changes to all the non-
.pyfiles. If possible, I think it would be ideal for reviewers to pull down the branch locally into a "fresh" directory and attempt to run through "common" dev workflows to ensure everything still works as intended.Also, it's worth noting that the CI workflow does install
uvandruffautomatically, but I don't think anything in any of the makefiles does the same for non-ci environments. Presumably that should be rectified prior to merging the branch 🤔? Reviewers, please chime in on it.