CI: Migrate seemethere/download-artifact-s3 locally #6157
Conversation
|
@jwagantall is attempting to deploy a commit to the Meta Open Source Team on Vercel. A member of the Team first needs to authorize it. |
facebook-github-bot
added
the
CLA Signed
|
Changes looks good to me in principle, but we should put some automation into building the actions and testing them, if they are part of test-infra on a personal repo |
jwagantall
force-pushed
the
migrate-download
branch
from
5a9482d to
788704e
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
Thank you so much for the review! Working on fixing the CI issues and linting issues. |
jwagantall
force-pushed
the
migrate-download
branch
3 times, most recently
from
507a669 to
6cd95dd
Compare
|
This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation. |
jwagantall
force-pushed
the
migrate-download
branch
from
6cd95dd to
4261640
Compare
jwagantall
force-pushed
the
migrate-download
branch
3 times, most recently
from
4010a09 to
2415e72
Compare
| - name: Upload artifact A | ||
| uses: actions/upload-artifact@v4 | ||
| with: | ||
| name: Artifact-A-${{ matrix.runs-on }} | ||
| path: path/to/artifact-A | ||
|
|
||
| - name: Upload artifact B | ||
| uses: actions/upload-artifact@v4 | ||
| with: | ||
| name: Artifact-B-${{ matrix.runs-on }} | ||
| path: path/to/artifact-B |
There was a problem hiding this comment.
These are the uploads that were in your original test to try the migrated download action.
Uploads are done using the original "actions/upload-artifact" action into GHA artifacts and then we call the migrated "download-artifact-s3" action to download these files. (Part of the unit test)
The code is pretty much the same compared to seemethere/download-artifact-s3 with the exception that I added the unique name so that it wont conflict with each run from the matrix definition.
There was a problem hiding this comment.
Um this is unnecessary, this tests the old action and not the new action so I would just remove them. No need to keep code that doesn't actually test the thing that we're trying to test.
| # Test downloading both artifacts at once | ||
| - name: Download all Artifacts | ||
| uses: ./.github/actions/download-artifact-s3 | ||
| with: | ||
| path: some/other/path | ||
|
|
||
| - name: Verify successful download | ||
| run: | | ||
| $fileA = "${{ github.workspace }}/some/other/path/Artifact-A-${{ matrix.runs-on }}/file-A.txt" | ||
| $fileB = "${{ github.workspace }}/some/other/path/Artifact-B-${{ matrix.runs-on }}/file-B.txt" | ||
| if(!(Test-Path -path $fileA) -or !(Test-Path -path $fileB)) | ||
| { | ||
| Write-Error "Expected files do not exist" | ||
| } | ||
| if(!((Get-Content $fileA) -ceq "Lorem ipsum dolor sit amet") -or !((Get-Content $fileB) -ceq "Hello world from file B")) | ||
| { | ||
| Write-Error "File contents of downloaded artifacts are incorrect" | ||
| } | ||
| shell: pwsh |
There was a problem hiding this comment.
Same as above. Is part of your original unit tests. I am only fixing the names to avoid errors due to conflicts with each of the runs.
jwagantall
force-pushed
the
migrate-download
branch
from
2415e72 to
dacff6c
Compare
jwagantall
force-pushed
the
migrate-download
branch
from
dacff6c to
2c123b7
Compare
| path: ~/some/path/with/a/tilde | ||
|
|
||
| - name: Verify successful download | ||
| run: | |
Check failure
Code scanning / lintrunner
ACTIONLINT/[shellcheck]
Hosting the code for seemethere/download-artifact-s3 locally to encorage more coolaboration and maintainance of this action. This first migration is based on v4.2.0 Issue: pytorch/ci-infra#242 Signed-off-by: Jessica Wagantall <[email protected]>
jwagantall
force-pushed
the
migrate-download
branch
from
2c123b7 to
7e84f64
Compare
Hosting the code for seemethere/download-artifact-s3 locally to encorage more coolaboration and maintainance of this action.
This first migration is based on v4.2.0
Issue: pytorch/ci-infra#242