oidc-exchange: warn on reusable workflow - #306
Conversation
Signed-off-by: William Woodruff <william@trailofbits.com>
| WPS102, | ||
| WPS110, | ||
| WPS111, | ||
| WPS202, |
There was a problem hiding this comment.
NB: I disabled this check because it's noisy on what (IMO) are reasonable levels of complexity within files: it's unhappy that oidc-exchange.py contains 9 > 7 module members, but I think decomposing those members further would make the code harder, not easier to read.
I can re-enable this and figure out a workaround, but I figured I'd leave a rationale here 🙂
There was a problem hiding this comment.
@woodruffw I prefer rationale as a code comment where it's disabled. Though, ideally, # noqas should be used in place rather than disabling rules globally. There's also per-file ignores in flake8. But if the complexity level is known to be higher across the project, there's --max-module-members that can be increased instead of disabling the rule too: https://wemake-python-styleguide.readthedocs.io/en/latest/pages/usage/violations/complexity.html#wemake_python_styleguide.violations.complexity.TooManyModuleMembersViolation
There was a problem hiding this comment.
As for decomposing the members, I don't think it'd make it harder to read. We just have to choose wisely. For example, the helpers for interfacing with GHA (like messages/warnings) could as well be put into a shared module that more scripts would reuse. This wouldn't make it harder to reason about.
There was a problem hiding this comment.
For example, the helpers for interfacing with GHA (like messages/warnings) could as well be put into a shared module that more scripts would reuse. This wouldn't make it harder to reason about.
That's true, although in that case I think we need to switch to a whole package structure here, rather than just Python files in the repo root 🙂
There was a problem hiding this comment.
Yeah, I was going to look into it at some point. Although, having it in the root as the first step isn't half-bad. We may need to set a PYTHONPATH or turn the invocation into a python -m modulename for it to be importable until the structure is changed.
|
|
||
|
|
||
| def render_claims(token: str) -> str: | ||
| def extract_claims(token: str) -> dict[str, typing.Any]: |
There was a problem hiding this comment.
Would object work here?
| def extract_claims(token: str) -> dict[str, typing.Any]: | |
| def extract_claims(token: str) -> dict[str, object]: |
I learned recently that MyPy docs recommend this instead of Any, whenever possible.
There was a problem hiding this comment.
I think so! TIL they recommend that, I've been using Any for years.
There was a problem hiding this comment.
I've been exposed to more MyPy things this year, between observing what the pytest and aiohttp co-maintainers do, and experimenting a lot with MyPy coverage reports which I didn't know were a thing a few years ago — I even started uploading those to Codecov and will hopefully bring that practice here.
@woodruffw could you also apply object to other places that I didn't explicitly mark?
| For more information, see: | ||
|
|
||
| * https://docs.pypi.org/trusted-publishers/troubleshooting/#reusable-workflows-on-github | ||
| * https://github.com/pypa/gh-action-pypi-publish/issues/166 |
There was a problem hiding this comment.
I was thinking about asking the users to subscribe for updates. Can you think of some wording to communicated that?
| Reusable workflows are **not currently supported** by PyPI's Trusted Publishing | ||
| functionality, and are subject to breakage. Users are **strongly encouraged** | ||
| to avoid using reusable workflows for Trusted Publishing until support | ||
| becomes official. |
There was a problem hiding this comment.
Would
| becomes official. | |
| becomes official. Please, do not report bugs if this breaks. |
make this message stronger?
| # A reusable workflow is identified by having different values | ||
| # for its workflow_ref (the initiating workflow) and job_workflow_ref | ||
| # (the reusable workflow). | ||
| if claims.get('workflow_ref') == claims.get('job_workflow_ref'): |
There was a problem hiding this comment.
Perhaps, store these into vars and reuse below?
Co-authored-by: 🇺🇦 Sviatoslav Sydorenko (Святослав Сидоренко) <wk.cvs.github@sydorenko.org.ua>
|
@woodruffw still WIP? |
|
Yes, sorry -- I haven't had the cycles to push this forwards recently. I'll try and circle back to it this coming week! |
|
No problem! Just trying to see if there's anything I should be doing here and if it's not forgotten :) |
|
@woodruffw are you able to get back to this? Should we mark it as a draft? |
Yeah, this popped off my stack again. I'll redraft until I have time to visit it. |
github-project-automation
Bot
moved this from 🧐 @webknjaz's review queue 📋
to 🌈 Done 🦄
in 📅 Procrastinating in public 😵💫
|
FTR, I figured this was important enough to include in v1.13.0 so I added those changes myself. |
2 participants
WIP; haven't tested this yet.
See #305 (comment).