Adding monorepo configurations to TheRock#695
Conversation
…min12/monorepo-therock-test
geomin12
changed the title
| - name: Upload Artifacts | ||
| run: | | ||
| aws s3 cp build/artifacts/ s3://therock-artifacts/${{github.run_id}}-linux/ \ | ||
| aws s3 cp build/artifacts/ s3://${{env.BUCKET}}/${{github.run_id}}-linux/ \ |
There was a problem hiding this comment.
currently, for external repos (ex: rocm-libraries), this still uploads all to the dir /.
After this PR, I plan to move this upload items into script in another PR and adding env.REPO to there to allow therock-external/rocm-libraries/123456-linux
There was a problem hiding this comment.
Thanks for the explanation - def a good thing we need to fix.
There was a problem hiding this comment.
Would therock-external correspond to env.BUCKET and rocm-libraries to env.REPO then? From a naming perspective, if env.REPO just specifies a subdir in the bucket you could call it env.S3_SUBDIR to make the purpose more clear.
There was a problem hiding this comment.
Correct! Although repo is not used (yet) so I will add this in the next PR!
|
Biggest concern is adding another bucket if we can avoid it - getting too many buckets as is. |
| PLATFORM = platform.system().lower() | ||
|
|
||
| BUCKET = os.getenv("BUCKET", "therock-artifacts") | ||
| REPO = os.getenv("REPO", "") |
There was a problem hiding this comment.
REPO seems unused in this script.
| - name: Upload Artifacts | ||
| run: | | ||
| aws s3 cp build/artifacts/ s3://therock-artifacts/${{github.run_id}}-linux/ \ | ||
| aws s3 cp build/artifacts/ s3://${{env.BUCKET}}/${{github.run_id}}-linux/ \ |
There was a problem hiding this comment.
Would therock-external correspond to env.BUCKET and rocm-libraries to env.REPO then? From a naming perspective, if env.REPO just specifies a subdir in the bucket you could call it env.S3_SUBDIR to make the purpose more clear.
| repository: "ROCm/TheRock" | ||
|
|
||
| - name: "Checking out repository for rocm-libraries" | ||
| if: ${{ github.repository == 'ROCm/rocm-libraries' }} |
There was a problem hiding this comment.
With view on more monorepos we might want to make this more flexible but also need take care of PRs from forks (either to one of the monorepos or TheRock). Maybe add a TODO note for now?
There was a problem hiding this comment.
I could actually make it github.event.repository.name == TheRock in order to work with both forks + normal so I will do that!
There was a problem hiding this comment.
Happy if that works for all the potential use cases. I think we need to cover PRs
- from within TheRock
- from forks of TheRock targeting TheRock
- form within a monorepo (tested via TheRock)
- form forks of a monorepo targeting the monorepo (tested via TheRock)
I am not familiar enough with GH's event API thus not saying that what you plan doesn't work :)
There was a problem hiding this comment.
That should work I expect.
|
I'm good with this once you are ready to remove the geo parts. Just poke me for an approval |
3 participants
Changes:
therock-artifacts-externalbucketProgress on #665