GHA Updates #186
GHA Updates #186
Conversation
lishaduck
force-pushed
the
gha
branch
3 times, most recently
from
1a42dbe
to
f4f9e4a
Compare
| permissions: | ||
| contents: write | ||
|
|
There was a problem hiding this comment.
I read through the dillonkearns/elm-publish-action source code, and I think this is all it needs.
There was a problem hiding this comment.
Alright. Next time I try to publish a package I'll try to backport this change there and test it out.
|
This doesn't resolve all of #173, but the other updates were more involved, while this is self-contained (and I'm on vacation). |
| # Checks-out your repository under $GITHUB_WORKSPACE, so your job can access it | ||
| - uses: actions/checkout@v4 | ||
| # Checks-out your repository under $GITHUB_WORKSPACE, so your job can access it | ||
| - uses: actions/checkout@692973e3d937129bcbf40652eb9f2f61becf3332 # v4.1.7 |
There was a problem hiding this comment.
Is @4 not enough? Do you think it risks breaking in the future? We could also miss on some security patches.
Some of these files will also be generated for new packages, and I don't imagine the users will want to update/upgrade the versions specified here all that often.
There was a problem hiding this comment.
I guess for generated files, it might be reasonable not to pin them, but for us, it's more secure not to.
Pinning to a commit means that a malicious actor can't change the tag, as a commit is immutable, but not a tag. GitHub also doesn't have the greatest track record with both bugs and SemVer, and has happily broken the latest versions of actions for weeks at a time.
Dependabot should bump them, so missing patches shouldn't be a major issue.
I explained this a few weeks ago in a similar PR, so if my wording doesn't make sense (I personally doubt I said this in a way that makes sense), try this explanation instead: material-extensions/vscode-material-icon-theme#2365 (comment).
There was a problem hiding this comment.
What do you think about (in a followup PR) adding pinning these versions in this file and in new-package generated files, and adding a comment to explain the reason and how to upgrade (how/where to find the new commit for instance). I think that way both this project and users of the package benefit from the improvements here.
There was a problem hiding this comment.
They're already pinned. Are you proposing to add a comment about why they're pinned?
EDIT: If so, we could just add a dependabot config in the github dir by default.
There was a problem hiding this comment.
Yes, a comment on how they're pinned, and how to upgrade to a new version. Right now I could ask you, but one day you might not (and new-package users probably won't either 😅 ).
I'm still on the fence for dependendabot. Honestly, it's not that useful for Elm packages, it would just create a bunch of noise for package auhors. I think it makes sense for this project (although I'm still trying to figure out whether it's too much noise or not).
There was a problem hiding this comment.
Yeah. That was a lot more noise than I'd expected.
With elm packages though, it should just notify that there're new majors.
There was a problem hiding this comment.
Yeah. That was a lot more noise than I'd expected.
Same here 😅
The GitHub rate limit has exploded 😂
With elm packages though, it should just notify that there're new majors.
Hmm, I don't even know if it should. For a package author, as long as they can run tools and tests (locally and in CI) and publish (through CI), they don't need to have the latest version of anything 🤷♂️
There was a problem hiding this comment.
Yeah, but it would block the application from upgrading other review rules.
Say elm-review-simplify needs elm-review 2 or 3, but only declares support for 2, and elm-review-unused needs elm-review 3, then one couldn't get the improvements to elm-review-unused.
(Probably some other package than elm-review, but I think my point is clear enough as is?)
There was a problem hiding this comment.
Another thing, I think I forgot to set it to weekly, I usually have it set to daily, but weekly is a lot more manageable in my experience.
EDIT: I did set it to daily.
|
Thank you! |
Way back a long time ago in jfmengels#186 (comment), @jfmengels asked me to write some comments. I also updated the workflows to align with recent changes to our test workflow in jfmengels#288.
Way back a long time ago in jfmengels#186 (comment), @jfmengels asked me to write some comments. I also updated the workflows to align with recent changes to our test workflow in jfmengels#288.
Way back a long time ago in jfmengels#186 (comment), @jfmengels asked me to write some comments. I also updated the workflows to align with recent changes to our test workflow in jfmengels#288.
Way back a long time ago in #186 (comment), @jfmengels asked me to write some comments. I also updated the workflows to align with recent changes to our test workflow in #288.
Improving security, etc.
Related: #173