Skip to content

Add configure-aws-credentials composite workflow with release support#4386

Merged
ScottTodd merged 4 commits into
mainfrom
users/scotttodd/s3-iam-lookup
Apr 8, 2026
Merged

Add configure-aws-credentials composite workflow with release support#4386
ScottTodd merged 4 commits into
mainfrom
users/scotttodd/s3-iam-lookup

Conversation

@ScottTodd
Copy link
Copy Markdown
Member

@ScottTodd ScottTodd commented Apr 7, 2026

Motivation

For multi-arch release workflows (#3334), I want to reuse the existing workflows like .github/workflows/multi_arch_build_portable_linux_artifacts.yml but have them upload to therock-dev-artifacts, therock-nightly-artifacts, etc. instead of therock-ci-artifacts. The _retrieve_bucket_info() function in build_tools/_therock_utils/workflow_outputs.py already selects the right bucket based on the RELEASE_TYPE environment variable, but this also requires switching the role from

-role-to-assume: arn:aws:iam::692859939525:role/therock-ci
+role-to-assume: arn:aws:iam::692859939525:role/therock-dev

I decided to take the chance to translate our s3_buckets.md into a s3_buckets.py file with two users:

  • The existing _retrieve_bucket_info() function in build_tools/_therock_utils/workflow_outputs.py
  • A new build_tools/github_actions/write_artifacts_bucket_info.py script used by a .github/actions/configure_aws_artifacts_credentials/action.yml composite workflow that handles looking up the role and calling aws-actions/configure-aws-credentials (with special-characters-workaround set on Windows)

Technical Details

This is in contrast to what was recently done for native linux packages (which we can migrate in a follow-up):

- name: Determine IAM role
id: iam_role
run: |
# ================================================================
# IAM Role Selection Logic
# ================================================================
# Determines which AWS IAM role to assume based on job_type from s3_config step.
#
# Role Mapping:
# ├─ IF job_type == "ci"
# │ └─ Use: therock-ci role
# │ (For all CI buckets: therock-ci-artifacts, therock-ci-artifacts-external, therock-artifacts-internal)
# │
# └─ ELSE (job_type == dev/nightly/prerelease/release)
# └─ Use: therock-${job_type} role
# (For package buckets: therock-dev-packages, therock-nightly-packages, etc.)
#
# ================================================================
JOB_TYPE="${{ steps.s3_config.outputs.job_type }}"
if [[ "${JOB_TYPE}" == "ci" ]]; then
# CI builds use the shared CI role (for all artifact buckets)
IAM_ROLE="arn:aws:iam::692859939525:role/therock-ci"
echo "✓ Using CI role: ${IAM_ROLE}"
else
# Release builds use release-type-specific roles (for package buckets)
IAM_ROLE="arn:aws:iam::692859939525:role/therock-${JOB_TYPE}"
echo "✓ Using release-type role: ${IAM_ROLE}"
fi
echo "iam_role=${IAM_ROLE}" >> $GITHUB_OUTPUT
- name: Configure AWS Credentials
uses: aws-actions/configure-aws-credentials@8df5847569e6427dd6c4fb1cf565c83acfa8afa7 # v6.0.0
with:
aws-region: us-east-2
role-to-assume: ${{ steps.iam_role.outputs.iam_role }}

We could put similar code directly in the composite workflow instead of adding the python scripts, but this is significantly easier to test and then match the behavior between different workflows and scripts that want to interface with the buckets.

Test Plan

  • Unit tests
  • CI on this PR (which should authenticate with therock-ci and upload to therock-ci-artifacts)
  • Watch CI on PRs from forks (which should skip authentication and upload to therock-ci-artifacts-external as before)

Test Result

Composite action worked: https://github.com/ROCm/TheRock/actions/runs/24109830592/job/70341723332?pr=4386#step:15:1

Submission Checklist

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
@ScottTodd ScottTodd marked this pull request as ready for review April 7, 2026 23:54
@ScottTodd ScottTodd requested a review from marbre April 7, 2026 23:54
@ScottTodd ScottTodd requested a review from geomin12 April 7, 2026 23:57
Comment on lines -172 to -175
uses: aws-actions/configure-aws-credentials@8df5847569e6427dd6c4fb1cf565c83acfa8afa7 # v6.0.0
with:
aws-region: us-east-2
role-to-assume: arn:aws:iam::692859939525:role/therock-ci
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

We could also update

- name: Configure AWS Credentials
if: ${{ github.repository_owner == 'ROCm' && !cancelled() }}
uses: aws-actions/configure-aws-credentials@8df5847569e6427dd6c4fb1cf565c83acfa8afa7 # v6.0.0
with:
aws-region: us-east-2
role-to-assume: arn:aws:iam::692859939525:role/therock-${{ env.RELEASE_TYPE }}

but that workflow uses more than just the therock-{release_type}-artifacts bucket, it also uses

S3_BUCKET_PY: "therock-${{ needs.setup_metadata.outputs.release_type }}-python"

For multi-arch release workflows, I plan on having the existing build workflows upload artifacts,logs,etc. to therock-{release_type}-artifacts and then have new workflow code copy from those buckets to e.g. therock-{release_type}-tarball


By the way, I decided to NOT update

- name: Configure AWS Credentials
if: ${{ github.repository == 'ROCm/TheRock' }}
uses: aws-actions/configure-aws-credentials@8df5847569e6427dd6c4fb1cf565c83acfa8afa7 # v6.0.0
with:
aws-region: us-east-2
role-to-assume: arn:aws:iam::692859939525:role/therock-ci

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Updating release_portable_linux_packages.yml should be fine as the role allows to access the other buckets as well. Can be in a follow up however.

Comment thread build_tools/tests/s3_buckets_test.py Outdated
self.assertEqual(config.name, "therock-ci-artifacts")

def _write_event(self, event: dict) -> str:
f = tempfile.NamedTemporaryFile(mode="w", suffix=".json", delete=False)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

From my agent's review:

The event-payload fork detection tests use tempfile and finally: os.unlink(...) - fine, but could use tempfile.NamedTemporaryFile(delete=True) with a context manager instead of manual cleanup.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

tempfile.NamedTemporaryFile(delete=True) won't work on Windows, as the file can't be reopened for read by another command while already opened by the context manager:

     >       with open(event_path) as f:
                  ^^^^^^^^^^^^^^^^
     E       PermissionError: [Errno 13] Permission denied:
     'C:\\Users\\NOD-SH~1\\AppData\\Local\\Temp\\tmpjr8p04dq.json'

We could use tempfile.TemporaryDirectory() though:

with tempfile.TemporaryDirectory() as tmpdir:
    event_path = Path(tmpdir) / "event.json"
    event_path.write_text(
        json.dumps({"pull_request": {"head": {"repo": {"fork": True}}}})
    )

How about we keep the NamedTemporaryFile and manual unlink() but add more comments? That matches recently added test code in https://github.com/ROCm/TheRock/blob/main/build_tools/github_actions/tests/configure_multi_arch_ci_test.py more closely too.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Oh we can also keep as is for consistency :)

Comment on lines -172 to -175
uses: aws-actions/configure-aws-credentials@8df5847569e6427dd6c4fb1cf565c83acfa8afa7 # v6.0.0
with:
aws-region: us-east-2
role-to-assume: arn:aws:iam::692859939525:role/therock-ci
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Updating release_portable_linux_packages.yml should be fine as the role allows to access the other buckets as well. Can be in a follow up however.

Comment on lines +34 to +35
iam_namespace: str | None = field(default="arn:aws:iam::692859939525:role")
"""Namespace for write_access_iam_role (e.g. 'arn:aws:iam::692859939525:role')"""
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

arn:aws:iam actually is the prefix, partition, service, which will always be the same here. 692859939525 is the account ID. We could also just store stat one. And should role be deduced from iam_role?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

How's this now?

ScottTodd and others added 2 commits April 8, 2026 09:01
NamedTemporaryFile(delete=True) holds an exclusive lock on Windows,
preventing the code under test from opening the file. Use delete=False
with try/finally cleanup, matching the pattern in
configure_multi_arch_ci_test.py.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Member

@marbre marbre left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for iterating.

@ScottTodd ScottTodd merged commit 1e7f885 into main Apr 8, 2026
88 of 91 checks passed
@ScottTodd ScottTodd deleted the users/scotttodd/s3-iam-lookup branch April 8, 2026 20:06
@github-project-automation github-project-automation Bot moved this from TODO to Done in TheRock Triage Apr 8, 2026
rahulc-gh pushed a commit that referenced this pull request Apr 9, 2026
…#4386)

## Motivation

For multi-arch release workflows
(#3334), I want to reuse the
existing workflows like
`.github/workflows/multi_arch_build_portable_linux_artifacts.yml` but
have them upload to `therock-dev-artifacts`,
`therock-nightly-artifacts`, etc. instead of `therock-ci-artifacts`. The
`_retrieve_bucket_info()` function in
`build_tools/_therock_utils/workflow_outputs.py` already selects the
right bucket based on the `RELEASE_TYPE` environment variable, but this
also requires switching the role from
```diff
-role-to-assume: arn:aws:iam::692859939525:role/therock-ci
+role-to-assume: arn:aws:iam::692859939525:role/therock-dev
```

I decided to take the chance to translate our
[s3_buckets.md](https://github.com/ROCm/TheRock/blob/main/docs/development/s3_buckets.md)
into a s3_buckets.py file with two users:
* The existing `_retrieve_bucket_info()` function in
`build_tools/_therock_utils/workflow_outputs.py`
* A new `build_tools/github_actions/write_artifacts_bucket_info.py`
script used by a
`.github/actions/configure_aws_artifacts_credentials/action.yml`
composite workflow that handles looking up the role and calling
`aws-actions/configure-aws-credentials` (with
`special-characters-workaround` set on Windows)

## Technical Details

This is in contrast to what was recently done for native linux packages
(which we can migrate in a follow-up):
https://github.com/ROCm/TheRock/blob/aee23850c29ad0f47e9f5a1ba494af54b25e23cf/.github/workflows/multi_arch_build_native_linux_packages.yml#L100-L131
https://github.com/ROCm/TheRock/blob/aee23850c29ad0f47e9f5a1ba494af54b25e23cf/.github/workflows/multi_arch_build_native_linux_packages.yml#L169-L173

We could put similar code directly in the composite workflow instead of
adding the python scripts, but this is significantly easier to test and
then match the behavior between different workflows and scripts that
want to interface with the buckets.

## Test Plan

* Unit tests
* CI on this PR (which should authenticate with `therock-ci` and upload
to `therock-ci-artifacts`)
* Watch CI on PRs from forks (which should skip authentication and
upload to `therock-ci-artifacts-external` as before)

## Test Result

Composite action worked:
https://github.com/ROCm/TheRock/actions/runs/24109830592/job/70341723332?pr=4386#step:15:1

## Submission Checklist

- [x] Look over the contributing guidelines at
https://github.com/ROCm/ROCm/blob/develop/CONTRIBUTING.md#pull-requests.

---------

Co-authored-by: Claude <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants