-
Notifications
You must be signed in to change notification settings - Fork 161
Fix packaging checks and simplify CI #1302
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
base: master
Are you sure you want to change the base?
Conversation
- name: Test with tox | ||
run: | | ||
tox --parallel 0 | ||
run: mkdir bin && curl -sSL "https://github.com/summerwind/h2spec/releases/latest/download/h2spec_linux_amd64.tar.gz" | tar -xz --directory ./bin && ./bin/h2spec --version |
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.
Does this always work? The original intention of calling the GitHub API using a token was to get around rate-limiting.
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.
Basically this is not using any API endpoint or raw file access, but the convenience public link for releases — that unless flagged by their security is generally unlimited. (They have a ~5k hits/hr limit however I think they don't limit access from their own infra to stuff like packages, containers or releases?) — I'll check the frequency but we've been using this in rust workflows (instead of waiting for cargo builds) for many years, can't remember it ever failing over a rate limit in GHA.
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.
@Kriechi (Just to make it more traceable: the CI never had used the public release URL, it only ever used the API, unauthenticated: a.) GitHub@9df8f94
, b.) Travis@0cb4090
… so I don't believe the non-APIv3 way was ever used.)
Not sure about the ruff/mypy typing related changes in this PR. The last commit to the master branch tested cleanly. Could you please shed some light on why you proposed this part? If relevant, please split it out into a dedicated PR for discussion. |
@Kriechi Same here, but also packaging tests passed back then — and now they'd fail [e.g.], probably based on some downstream version bumps in dependencies introducing bugs. I'd much rather avoid the cast quoting myself, but the checks won't pass otherwise [e.g.]. Would you like me to add |
(NB: On last master CI run Feb 27, "ruff>=0.8.0,<1" collected ruff==0.9.8, today it resolves to ruff==0.11.7 👀 … The |
So, another option here might be, instead of adding the rule to ignorelist, to constrain ruff a bit more, and find a version that's comfortable for now, e.g. |
Co-authored-by: Jan Brasna <1784648+janbrasna@users.noreply.github.com>
I prefer going with a reasonably "latest" version of ruff et al. So let's keep the version pin as is. Not sure I understand the twine changes here yet - are they related to the ruff dependency? If not, please also split them out - unless thats the only thing remaining in this PR here. |
Due to a bug in
twine<6.0.1
not reading the correct metadata using wildcards thus failing thetox -e packaging
steps, I've moved the pin from5.1.x
to6.1.x
to include some additional compatibility with packaging dependencies.This also bumps outdated
actions/*
and generally simplifies and speeds up the workflow.Inlining the
h2spec
step also allows for getting rid of its install script and exposing the access token.NB: To currently pass
ruff
/mypy
checks, I had to fixruntime-cast-value
(TC006) by quoting the type expressions inh2/stream
occurrences ofcast()
…Logs:
actions/runs/14669502174