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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 13 commits into from Mar 13, 2024
Merged

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

merged 13 commits into from Mar 13, 2024

Conversation

ramonpetgrave64
Copy link
Collaborator

@ramonpetgrave64 ramonpetgrave64 commented Mar 8, 2024 •
edited

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 force-pushed the ramonpetgrave64-upload-download-artifact-v4 branch from 14806b8 to 8909854 Compare March 8, 2024 18:35
@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
@@ -99,6 +100,10 @@ duplication."

## Unreleased

### 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

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
Collaborator Author

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.

.github/workflows/pre-submit.actions.yml Outdated Show resolved Hide resolved
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
Collaborator Author

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

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
Collaborator Author

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

@laurentsimon
Copy link
Collaborator

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

@ramonpetgrave64
Copy link
Collaborator Author

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

  • 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

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
72 of 74 checks passed
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 joshuagl is a code owner

@kpk47 kpk47 Awaiting requested review from kpk47 kpk47 is a code owner

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