Conversation
There was a problem hiding this comment.
Pull request overview
Adds AWS S3 publishing plumbing to the FlyDSL GitHub Actions CI workflow, aiming to distribute nightly wheel builds via OIDC-authenticated AWS credentials.
Changes:
- Added job-level
id-token: writepermissions and steps to configure AWS credentials via OIDC. - Added an AWS CLI installation script intended for CI runners.
- Added a (currently stubbed) step intended to publish wheel artifacts to an S3 bucket path.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
scripts/install_awscli.sh |
New helper script to download/install AWS CLI v2 on Linux CI runners. |
.github/workflows/flydsl.yaml |
Adds OIDC permissions and AWS credential/config/publish steps to the FlyDSL CI workflow. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - name: Publish packages to S3 | ||
| if: ${{ always() && !github.event.pull_request.head.repo.fork }} | ||
| run: | | ||
| # aws s3 cp ${{ github.workspace }}/dist/ s3://framework-whls-nightlies/flydsl/ --recursive --exclude "*" --include "*.whl" | ||
|
|
There was a problem hiding this comment.
The publish step is currently a no-op (the aws s3 cp command is commented out), so no wheels will be uploaded despite the PR goal. Also, the source path uses ${{ github.workspace }}/dist/, but this workflow checks out the repo into flydsl-test/, so wheel artifacts would typically be under ${{ github.workspace }}/flydsl-test/dist/ (and there is no step here that actually builds wheels into dist/). Please add/enable the wheel build step (e.g., run the existing wheel build script) and upload from the correct dist directory.
| if: ${{ always() && !github.event.pull_request.head.repo.fork }} | ||
| uses: aws-actions/configure-aws-credentials@v4 | ||
| with: | ||
| aws-region: us-east-1 | ||
| role-to-assume: arn:aws:iam::661452401056:role/framework-flydsl-nightlies | ||
|
|
||
| - name: Install AWS CLI | ||
| if: always() | ||
| run: bash ./flydsl-test/scripts/install_awscli.sh | ||
|
|
||
| - name: Publish packages to S3 | ||
| if: ${{ always() && !github.event.pull_request.head.repo.fork }} |
There was a problem hiding this comment.
always() here means AWS role assumption / publishing can run even when tests or builds fail, and it will also run on pull_request events for branches within the same repo (non-forks). For an artifact publishing role, it’s safer to gate this to successful runs on trusted events only (e.g., push to main and/or workflow_dispatch) to avoid uploading unreviewed PR artifacts or partial/failed outputs.
| if: ${{ always() && !github.event.pull_request.head.repo.fork }} | |
| uses: aws-actions/configure-aws-credentials@v4 | |
| with: | |
| aws-region: us-east-1 | |
| role-to-assume: arn:aws:iam::661452401056:role/framework-flydsl-nightlies | |
| - name: Install AWS CLI | |
| if: always() | |
| run: bash ./flydsl-test/scripts/install_awscli.sh | |
| - name: Publish packages to S3 | |
| if: ${{ always() && !github.event.pull_request.head.repo.fork }} | |
| if: ${{ success() && (github.event_name == 'workflow_dispatch' || (github.event_name == 'push' && github.ref == 'refs/heads/main')) }} | |
| uses: aws-actions/configure-aws-credentials@v4 | |
| with: | |
| aws-region: us-east-1 | |
| role-to-assume: arn:aws:iam::661452401056:role/framework-flydsl-nightlies | |
| - name: Install AWS CLI | |
| if: ${{ success() && (github.event_name == 'workflow_dispatch' || (github.event_name == 'push' && github.ref == 'refs/heads/main')) }} | |
| run: bash ./flydsl-test/scripts/install_awscli.sh | |
| - name: Publish packages to S3 | |
| if: ${{ success() && (github.event_name == 'workflow_dispatch' || (github.event_name == 'push' && github.ref == 'refs/heads/main')) }} |
| if: always() | ||
| run: bash ./flydsl-test/scripts/install_awscli.sh |
There was a problem hiding this comment.
This installs AWS CLI on the runner on every run (if: always()), even when publishing is skipped. On self-hosted runners this mutates the host environment across jobs and can introduce flaky behavior/version drift. Consider installing into a job-local directory (or using an action-provided AWS CLI) and gate the step to the same condition as publishing (and ideally only after wheel build succeeds).
| if: always() | |
| run: bash ./flydsl-test/scripts/install_awscli.sh | |
| if: ${{ always() && !github.event.pull_request.head.repo.fork }} | |
| uses: aws-actions/aws-cli-setup@v4 |
| curl --silent --fail --show-error --location \ | ||
| "https://awscli.amazonaws.com/awscli-exe-linux-${ARCH}.zip" \ | ||
| --output "awscliv2.zip" | ||
|
|
||
| unzip -qq awscliv2.zip | ||
|
|
||
| if [ "$EUID" -ne 0 ]; then | ||
| sudo ./aws/install --update | ||
| else | ||
| ./aws/install --update | ||
| fi |
There was a problem hiding this comment.
This script installs AWS CLI system-wide via ./aws/install --update and leaves the downloaded zip/extracted aws/ directory behind. On CI (especially self-hosted runners) that can cause cross-job side effects and workspace bloat. Consider installing into a temporary directory (e.g., under $RUNNER_TEMP) with a trap-based cleanup, and exposing it via PATH for the remainder of the job instead of updating the host installation.
|
|
@kiran-thumma You mentioned this is for nightly wheel builds, but these changes will publish packages in every PR tests? |
|
still failed @kiran-thumma |
gyohuangxin
left a comment
There was a problem hiding this comment.
LGTM, just a minor question.
| echo "logs.tgz not found; skipping log extraction" | ||
| fi | ||
|
|
||
| - name: Configure AWS credentials |
There was a problem hiding this comment.
Need we still need those steps in PR tests since we have a nightly one?
Motivation
Add AWS S3 publishing support to the FlyDSL CI workflow for distributing nightly wheel builds.
Technical Details
aws s3 cpto publish.whlartifacts tos3://framework-whls-nightlies/flydsl/.id-token: writepermission for GitHub Actions OIDC federation with IAM roleframework-flydsl-nightlies.scripts/install_awscli.shfor portable AWS CLI installation on runners.Test Plan
linux-flydsl-mi325-1andlinux-flydsl-mi355-1runners and wheels are uploaded to S3.Test Result
Submission Checklist