Skip to content
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

Update github actions to the latest versions #1026

Merged
merged 5 commits into from
Sep 24, 2024

Conversation

yamt
Copy link
Contributor

@yamt yamt commented Sep 18, 2024

No description provided.

@geky-bot
Copy link
Collaborator

Tests passed ✓, Code: |, Stack: |, Structs: |
Code Stack Structs Coverage
Default Lines 2394/2574 lines (-0.0%)
Readonly Branches 1245/1584 branches (+0.0%)
Threadsafe Benchmarks
Multiversion Readed 29369693876 B (+0.0%)
Migrate Proged 1482874766 B (+0.0%)
Error-asserts Erased 1568888832 B (+0.0%)

@geky
Copy link
Member

geky commented Sep 19, 2024

Thanks for this, looks like @geky-bot is still having a bit of issue since we were relying on the original artifact merging behavior.

Will poke around and see if there's a fix.

@geky
Copy link
Member

geky commented Sep 19, 2024

Looks like there's a merge-on-download option: https:/actions/upload-artifact/blob/main/docs/MIGRATION.md#multiple-uploads-to-the-same-named-artifact

Will experiment, I have another fork of the repo for this. There's a few delicate steps (release scripts) that rely on artifacts.

@geky
Copy link
Member

geky commented Sep 19, 2024

Pushed up a couple changes to fix things, tested here: geky/littlefs-test-repo#10

Also tweaked a couple things while editing/testing this:

  • Switched from dawidd6/action-download-artifact to the standard actions/download-artifact for cross-workflow downloads.

    dawidd6/action-download-artifact used to be the only way to accomplish this, but it looks like the standard action added support for cross-workflow downloads recently.

  • Dropped minor/patch pinning of action version, so only pinning on major version.

    I'm not sure there's any benefit to pinning minor/patch versions. GitHub is rather aggressive at deprecating/breaking anything out-of-date, so pinning the minor/patch version is more a liability than anything.

    Learned this the hard way trying to pin the Ubuntu version.

Let me know if this looks good to you, and I can go ahead and merge this into the devel branch to unblock other PRs.

@geky-bot
Copy link
Collaborator

Tests passed ✓, Code: 17064 B (+0.0%), Stack: 1440 B (+0.0%), Structs: 812 B (+0.0%)
Code Stack Structs Coverage
Default 17064 B (+0.0%) 1440 B (+0.0%) 812 B (+0.0%) Lines 2394/2574 lines (-0.0%)
Readonly 6194 B (+0.0%) 448 B (+0.0%) 812 B (+0.0%) Branches 1245/1584 branches (+0.0%)
Threadsafe 17924 B (+0.0%) 1440 B (+0.0%) 820 B (+0.0%) Benchmarks
Multiversion 17124 B (+0.0%) 1440 B (+0.0%) 816 B (+0.0%) Readed 29369693876 B (+0.0%)
Migrate 18760 B (+0.0%) 1744 B (+0.0%) 816 B (+0.0%) Proged 1482874766 B (+0.0%)
Error-asserts 17748 B (+0.0%) 1432 B (+0.0%) 812 B (+0.0%) Erased 1568888832 B (+0.0%)

Turns out major versions break things.

Old behavior: Artifacts with same name are merged
New behavior: Artifacts with same name error

Using a pattern and merging on download should fix this at least on the
job-side. Though I do wonder if we'll start running into artifact limit
issues with the new way artifacts are handled...
Looks like cross-workflow downloads has finally been added to the
standard download-artifact action, so we might as well switch to it to
reduce dependencies.

dawidd6's version was also missing the merge-multiple feature which is
necessary to work around breaking changes in download-artifact's v4
bump.

Weirdly it needs GITHUB_TOKEN for some reason? Not sure why this
couldn't be implicit.
With GitHub forcibly deprecating old versions of actions, pinning the
minor/patch version is more likely to cause breakage than not.
@geky-bot
Copy link
Collaborator

Tests passed ✓, Code: 17064 B (+0.0%), Stack: 1440 B (+0.0%), Structs: 812 B (+0.0%)
Code Stack Structs Coverage
Default 17064 B (+0.0%) 1440 B (+0.0%) 812 B (+0.0%) Lines 2394/2574 lines (-0.0%)
Readonly 6194 B (+0.0%) 448 B (+0.0%) 812 B (+0.0%) Branches 1245/1584 branches (+0.0%)
Threadsafe 17924 B (+0.0%) 1440 B (+0.0%) 820 B (+0.0%) Benchmarks
Multiversion 17124 B (+0.0%) 1440 B (+0.0%) 816 B (+0.0%) Readed 29369693876 B (+0.0%)
Migrate 18760 B (+0.0%) 1744 B (+0.0%) 816 B (+0.0%) Proged 1482874766 B (+0.0%)
Error-asserts 17748 B (+0.0%) 1432 B (+0.0%) 812 B (+0.0%) Erased 1568888832 B (+0.0%)

@yamt
Copy link
Contributor Author

yamt commented Sep 21, 2024

Thanks for this, looks like @geky-bot is still having a bit of issue since we were relying on the original artifact merging behavior.

oops, i was not aware of the merging behavior.

@geky
Copy link
Member

geky commented Sep 21, 2024

To be fair it is pretty unintuitive behavior, but it was useful. I wonder if we'll see more issues with hitting the artifact limit with GitHub's changes.

@geky
Copy link
Member

geky commented Sep 24, 2024

Merging

@geky geky changed the base branch from master to devel September 24, 2024 17:23
@geky geky merged commit b78afe2 into littlefs-project:devel Sep 24, 2024
16 checks passed
@geky geky added the github GitHub being GitHub label Sep 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
github GitHub being GitHub next patch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants