Skip to content

fix: upload-artifact and download-artifact v4#3312

Merged
laurentsimon merged 13 commits intoslsa-framework:mainfrom
ramonpetgrave64:ramonpetgrave64-upload-download-artifact-v4
Mar 13, 2024
Merged

fix: upload-artifact and download-artifact v4#3312
laurentsimon merged 13 commits intoslsa-framework:mainfrom
ramonpetgrave64:ramonpetgrave64-upload-download-artifact-v4

Conversation

@ramonpetgrave64
Copy link
Contributor

@ramonpetgrave64 ramonpetgrave64 commented Mar 8, 2024 •
edited
Loading

Summary

Testing Process

This change is tested with our existing PR Check workflows that use both directly and indirectly call upload-artifact and download-artifact.

  • One test for secure-upload-folder should fail in this PR because it will use secure-upload-artifact@main. There's no workaround to dynamically use the PR's ref instead of @main, but after merging this PR, the test should start passing.

Checklist

  • Review the contributing guidelines
  • Add a reference to related issues in the PR description.
  • Update documentation if applicable.
  • Add unit tests if applicable.
  • Add changes to the CHANGELOG if applicable.

@ramonpetgrave64 ramonpetgrave64 changed the title fix: upload and download artifact v4 fix: upload-artifact and download-artifact artifact v4 Mar 8, 2024
@ramonpetgrave64 ramonpetgrave64 changed the title fix: upload-artifact and download-artifact artifact v4 fix: upload-artifact and download-artifact v4 Mar 8, 2024
@ramonpetgrave64
use upload-artifact and download-artifact v4
74fd215 
Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>
@ramonpetgrave64
npm audit
8756677 
Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>
@ramonpetgrave64
markdown lint
4488d71 
Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>
@ramonpetgrave64
checkout the PR version
c388888 
Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>
@ramonpetgrave64
conditional assignment
b16effd 
Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>
@ramonpetgrave64
fix attempt to use dynamic secure-uplaod-artifact
0898d42 
Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>
@ramonpetgrave64
Revert "fix attempt to use dynamic secure-uplaod-artifact"
8909854 
This reverts commit 90909d4.

Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>
@ramonpetgrave64 ramonpetgrave64 marked this pull request as ready for review March 8, 2024 18:40
laurentsimon
laurentsimon reviewed Mar 8, 2024
View reviewed changes
Copy link
Collaborator

@laurentsimon laurentsimon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Had a comment on the workflow update

CHANGELOG.md Outdated

### Unreleased: Breaking Change: upload-artifact and download-artifact

- Our workflows now use the new `@v4`s of `actions/upload-artifact` and `actions/download-artifact`, which are incompatiblle with the prior `@v3`. See Our docs on the [generic generator](./internal/builders/generic/README.md#compatibility-with-actionsdownload-artifact).
Copy link
Collaborator

@laurentsimon laurentsimon Mar 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Add a sentence (or a section) on how to upgrade from v2 to v3. I know it looks obvious, but let's call it out explicitly. (We can clean up the "how to upgrade" into. dedicated section when we do the release if needed) Wdut?

Copy link
Contributor Author

@ramonpetgrave64 ramonpetgrave64 Mar 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you're right. A little bit of extra docs can go a long way. Added.

laurentsimon
laurentsimon reviewed Mar 8, 2024
View reviewed changes
Copy link
Collaborator

@laurentsimon laurentsimon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason you ad to update the package-lock.json? That should not be necessary. Maybe a leftover from prior experiments :)?

@ramonpetgrave64
Copy link
Contributor Author

ramonpetgrave64 commented Mar 11, 2024

Any reason you ad to update the package-lock.json? That should not be necessary. Maybe a leftover from prior experiments :)?

Just a package that had a vulnerability. I bumped the version with npm audit fix.

@ramonpetgrave64
undo explicit params
fa0d6b5 
Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>
@ramonpetgrave64
upgrade guidance
5f377b1 
Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>
@ramonpetgrave64
more migration guidance
fe8272f 
Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>
@ramonpetgrave64 ramonpetgrave64 mentioned this pull request Mar 11, 2024
Merged
laurentsimon
laurentsimon approved these changes Mar 12, 2024
View reviewed changes
Copy link
Collaborator

@laurentsimon laurentsimon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks1

Can we do the package json update in a separate PR to avoid "polluting" this PR's changes?

@laurentsimon
Copy link
Collaborator

laurentsimon commented Mar 12, 2024

please check the linter warning

@ramonpetgrave64
markdownlint
ed8743d 
Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>
@ramonpetgrave64
undo package-lock
8794ec2 
Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>
@ramonpetgrave64
Copy link
Contributor Author

ramonpetgrave64 commented Mar 13, 2024

@laurentsimon I removed the npm change and fixed the makrdownlint errors

@laurentsimon
Copy link
Collaborator

laurentsimon commented Mar 13, 2024

secure-upload-folder seems to be failing. can you take a look?

@ramonpetgrave64
Copy link
Contributor Author

ramonpetgrave64 commented Mar 13, 2024

Yes, it should fail because

  • One test for secure-upload-folder should fail in this PR because it will use secure-upload-artifact@main. There's no workaround to dynamically use the PR's ref instead of @main, but after merging this PR, the test should start passing

@laurentsimon
Merge branch 'main' into ramonpetgrave64-upload-download-artifact-v4
ef72311 
Signed-off-by: laurentsimon <64505099+laurentsimon@users.noreply.github.com>
@laurentsimon laurentsimon enabled auto-merge (squash) March 13, 2024 20:28
@laurentsimon laurentsimon mentioned this pull request Mar 13, 2024
Open
@laurentsimon
Copy link
Collaborator

laurentsimon commented Mar 13, 2024

  • secure-upload-folder

Got it. I created #3320 for tracking. Should be easy to fix.

I'll disable the check temporarily to merge

@laurentsimon
Copy link
Collaborator

laurentsimon commented Mar 13, 2024 •
edited
Loading

Any idea why pre-submit e2e generic default / build / final (pull_request) is failing? I suppose it's related to the secure-upload folder...

@laurentsimon laurentsimon merged commit b595e06 into slsa-framework:main Mar 13, 2024
ramonpetgrave64 added a commit that referenced this pull request Mar 20, 2024
@ramonpetgrave64
Revert "fix: upload-artifact and download-artifact v4 (#3312)"
a64a9d3 
This reverts commit b595e06.
@ramonpetgrave64 ramonpetgrave64 mentioned this pull request Mar 20, 2024
Merged
laurentsimon pushed a commit that referenced this pull request Mar 20, 2024
@ramonpetgrave64
chore: Revert "fix: upload-artifact and download-artifact v4" (#3398)
90f2eb1 
Reverts #3312
#3393
ramonpetgrave64 added a commit that referenced this pull request Mar 20, 2024
@ramonpetgrave64
Revert "fix: upload-artifact and download-artifact v4 (#3312)"
93680b1 
This reverts commit b595e06.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@laurentsimon laurentsimon laurentsimon approved these changes

@joshuagl joshuagl Awaiting requested review from joshuagl

@kpk47 kpk47 Awaiting requested review from kpk47

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[feature] Use upload-artifact v4

2 participants

@ramonpetgrave64 @laurentsimon