Skip to content
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions build_tools/github_actions/github_actions_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -538,6 +538,14 @@ def retrieve_bucket_info(
If neither is provided, |is_pr_from_fork| is populated from the
IS_PR_FROM_FORK environment variable.

Environment (optional override):
S3_BUCKET_ARTIFACTS: When set, use this bucket for the artifacts bucket
instead of the default chosen from repository/fork/release_type.

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.

An override seems useful, but we need to be careful here. Environment variables are global, affecting all processes. Some of our workflows need the ability to read artifacts from one S3 bucket and write to another S3 bucket. We can do things like only set the environment variable for certain workflow steps, but at that point I think directly passing some setting through to the top level scripts and then into this function (or where I'm moving it to in #3596) would be safer.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Replaced the usage of an env with an explicit --artifacts-bucket arg that gets passed directly into WorkflowOutputRoot.from_workflow_run() as a bucket_override parameter.


Security: Bucket permissions are enforced by AWS IAM, not by this code.
When using S3_BUCKET_ARTIFACTS, ensure the role assumed by the workflow
(e.g. via OIDC) is scoped so it can write only to intended buckets.

Returns a tuple [EXTERNAL_REPO, BUCKET], where:
- EXTERNAL_REPO = if CI is run on an external repo, we create a S3 sub-folder
to avoid conflicting run IDs
Expand Down Expand Up @@ -584,6 +592,15 @@ def retrieve_bucket_info(
else f"{owner}-{repo_name}/"
)

# When set, use explicit artifacts bucket.
s3_bucket_artifacts = os.getenv("S3_BUCKET_ARTIFACTS", "").strip()
if s3_bucket_artifacts:
_log(" S3_BUCKET_ARTIFACTS is set, using explicit artifacts bucket")
_log("Retrieved bucket info:")
_log(f" external_repo: {external_repo}")
_log(f" bucket : {s3_bucket_artifacts}")
return (external_repo, s3_bucket_artifacts)

release_type = os.getenv("RELEASE_TYPE")
if release_type:
_log(f" (implicit) RELEASE_TYPE: {release_type}")
Expand Down
64 changes: 58 additions & 6 deletions build_tools/github_actions/post_build_upload.py

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.

This script is only used by "CI", not "Multi-arch CI", and thus is scheduled for deletion (#3340). New feature work should be in https://github.com/ROCm/TheRock/blob/main/build_tools/github_actions/post_stage_upload.py

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Unless NPI pipelines are changed to work with multi-arch CI, such scripts that the older CI used cannot be deleted. A discussion on how multi-arch CI can intersect NPI pipelines is overdue.

Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,20 @@
[--build-dir BUILD_DIR]
[--upload | --no-upload] (default enabled if the `CI` env var is set)
[--run-id RUN_ID]
[--s3-subdir-artifacts PREFIX] [--s3-summary-base-url URL] [--s3-summary-path PREFIX]
[--skip-manifest]

S3 path/URL arguments (--s3-subdir-artifacts, --s3-summary-base-url, --s3-summary-path):
Must not end with a forward slash. The script joins segments with "/".
--s3-subdir-artifacts is the path prefix in the S3 bucket; --s3-summary-path is
the corresponding path as it appears in the CloudFront (summary) URL. This
optional path aligns with the prerelease CloudFront layout used for tarballs
and wheels.

Example (CloudFront base URL for job summary links):
--s3-subdir-artifacts v3/artifacts
--s3-summary-base-url https://rocm.foobar.amd.com
--s3-summary-path artifacts

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.

I don't think we should be mentioning CloudFront at all in most of our code. It's a CDN, an implementation detail. What matters is the URL, which could be backed by any CDN or other webserver and this code would not change.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Removed mentions of CloudFront to support any CDN.


This script runs after building TheRock, where this script does:
1. Create log archives
Expand Down Expand Up @@ -309,10 +323,23 @@ def run(args):

external_repo_path, bucket = retrieve_bucket_info()
run_id = args.run_id
bucket_uri = f"s3://{bucket}/{external_repo_path}{run_id}-{PLATFORM}"
bucket_url = (
f"https://{bucket}.s3.amazonaws.com/{external_repo_path}{run_id}-{PLATFORM}"
path_suffix = (
f"{args.s3_subdir_artifacts}/{external_repo_path}{run_id}-{PLATFORM}"
if args.s3_subdir_artifacts
else f"{external_repo_path}{run_id}-{PLATFORM}"
)
bucket_uri = f"s3://{bucket}/{path_suffix}"

if args.s3_summary_base_url:
run_suffix = f"{external_repo_path}{run_id}-{PLATFORM}"
summary_path = (
f"{args.s3_summary_path}/{run_suffix}"
if args.s3_summary_path
else path_suffix
)
bucket_url = f"{args.s3_summary_base_url}/{summary_path}"
else:
bucket_url = f"https://{bucket}.s3.amazonaws.com/{path_suffix}"

log("Write Windows time sync log")
log("----------------------")
Expand All @@ -328,9 +355,10 @@ def run(args):
log("----------")
upload_logs_to_s3(args.artifact_group, args.build_dir, bucket_uri)

log("Upload manifest")
log("----------------")
upload_manifest_to_s3(args.artifact_group, args.build_dir, bucket_uri)
if not args.skip_manifest:
log("Upload manifest")
log("----------------")
upload_manifest_to_s3(args.artifact_group, args.build_dir, bucket_uri)

log("Write github actions build summary")
log("--------------------")
Expand Down Expand Up @@ -363,6 +391,30 @@ def run(args):
parser.add_argument(
"--job-status", type=str, help="Status of this Job ('success', 'failure')"
)
parser.add_argument(
"--s3-subdir-artifacts",
type=str,
default="",
help="S3 path prefix for artifacts (e.g. v3/artifacts). Must not end with /.",
)
parser.add_argument(
"--s3-summary-base-url",
type=str,
default="",
help="Base URL for job summary links (e.g. CloudFront). Must not end with /. If set, summary uses this instead of raw S3 URL.",
)
parser.add_argument(
"--s3-summary-path",
type=str,
default="",
help="Path prefix for summary URL when --s3-summary-base-url is set (e.g. artifacts). Must not end with /.",
)
Comment on lines +358 to +363

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.

Either strip trailing / characters or assert with an argument parsing error. Comments are not error handling.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Trailing / characters are now rstrip'd.

parser.add_argument(
"--skip-manifest",
action=argparse.BooleanOptionalAction,
default=False,
help="Skip manifest upload.",
)
args = parser.parse_args()

# Check preconditions for provided arguments before proceeding.
Expand Down
Loading