ci: Harden CI, signed-commit on maintainers, non-regression on next, workspace inheritance#2999
Conversation
3aff3f5 to
d19e65c
Compare
43dca03 to
838e2ad
Compare
| predicates = "3.1" | ||
| pretty_assertions = { version = "1.4", default-features = false } | ||
| proc-macro2 = "1.0" | ||
| pubgrub = "0.3" |
There was a problem hiding this comment.
It sort of looks like this got overly aggressive about promoting deps to workspace level - I'd only promote when there are multiple crates in the workspace with the same dependency, but this seems to promote if even one crate has a dependency, e.g. pubgrub
There was a problem hiding this comment.
Does something bad happen if all versioning info is managed in the workspace's Cargo.toml?
There was a problem hiding this comment.
We do something similar in the node to prevent the dependency from "escaping".
For example, we create some wrapper crate and it and only it should ever have the wrapped dependency. Having it be in the workspace root de facto marks it as being available for use at large.
There was a problem hiding this comment.
Having it be in the workspace root de facto marks it as being available for use at large.
I misunderstand what you're suggesting. There's no hard mechanism that I know of that a dependency being in a workspace root makes it "available", you still need to write down the dependency at the crate in a source change. Its version is the only thing which arguably has a specification shorthand.
You both seem that to culturally indicate a minimalistic convention for the inheritance check, i.e. you should be a workspace dependency if and only if used by >1 crate (IOW a --promotion-threshold 2 in the tool). I'd rather not bother with the "only if" but I care about the "if" being done, so I'll implement your additional restriction, LMK if I didn't get it.
There was a problem hiding this comment.
you still need to write down the dependency in a source change.
Yes, I'm just noting how we've informally used it to indicate a "global" vs "local" dependency. Its not enforceable or enforced in any way.
However, it is easier to overlook a dep_xxx = { workspace = true } addition than the dep_xxx = "a.b.c", or if adding it triggers CI to suggest making the (now) shared dep as a workspace dep. Its simply used as a marker.
There was a problem hiding this comment.
I don't need an absolute >1 requirement, I'm just saying sometimes we do currently isolate a dependency to a single crate for specific reasons that are not enforceable except as a convention of sorts.
There was a problem hiding this comment.
For me it is about making it clear whether a dependency is actually used by the whole workspace, or just by a specific crate - when everything is at the workspace level, it is a lot less clear.
| - name: Resolve author permission | ||
| id: perm | ||
| uses: actions/github-script@f28e40c7f34bde8b3046d885e986cb6290c5673b # v7.1.0 |
There was a problem hiding this comment.
I'd maybe comment why we need this
There was a problem hiding this comment.
Because it's a gigantic PITA to trigger this signed-commits check only on maintainers (as decided in the last all-hands meeting): Github's roles are overly broad since the majority of us do not get our repo permissions through a role on the repo, but through a team permission derived from the org instead. This probe fetches the write permissions that the PR's author has on the repo, ignoring the repo-wide role entirely, and reliably detecting who should be bugged about signing their commits..
There was a problem hiding this comment.
I do agree; I mean in the yml itself so one knows why we're doing this "weird thing"
1f162d7 to
88e65bd
Compare
|
re: CI failure, go stamp 0xMiden/.github#14 :) |
88e65bd to
be7ec48
Compare
be7ec48 to
39136c1
Compare
aec8733 to
8832c54
Compare
Tightens the repo’s GitHub Actions setup and keeps current workflow behavior the same (mostly).
pull_request_targetworkflows that need to comment on PRs.cargo-msrvat a fixed version with--locked.