Skip to content

[ci] Updating retrieve_bucket_info to pull artifacts from correct bucket based on run ID #2357

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
geomin12 merged 5 commits into from
Dec 5, 2025
Merged

[ci] Updating retrieve_bucket_info to pull artifacts from correct bucket based on run ID #2357

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d70cb92
Adding fix for older run ids
geomin12 Dec 1, 2025
7840e31
fixing unit tests
geomin12 Dec 1, 2025
402723e
Merge branch 'main' into users/geomin12/date-check-run-id
geomin12 Dec 1, 2025
fab2fea
Adding PR comment
geomin12 Dec 4, 2025
5652577
Merge branch 'users/geomin12/date-check-run-id' of https://github.com…
geomin12 Dec 4, 2025
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
15 changes: 15 additions & 0 deletions build_tools/github_actions/github_actions_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
See also https://pypi.org/project/github-action-utils/.
"""

from datetime import datetime, timezone
import json
import os
from pathlib import Path
Expand Down Expand Up @@ -195,6 +196,8 @@ def retrieve_bucket_info(

_log("Retrieving bucket info...")

curr_commit_dt = None

if github_repository:
_log(f" (explicit) github_repository: {github_repository}")
if not github_repository:
Expand All @@ -211,6 +214,14 @@ def retrieve_bucket_info(
is_pr_from_fork = head_github_repository != github_repository
_log(f" head_github_repository : {head_github_repository}")
_log(f" is_pr_from_fork : {is_pr_from_fork}")

# From TheRock #2046 onward, a new S3 bucket was used.
# This datetime comparison will determine whether to download from older bucket or newer bucket.
curr_commit_dt = datetime.strptime(
workflow_run["updated_at"], "%Y-%m-%dT%H:%M:%SZ"
)
curr_commit_dt = curr_commit_dt.replace(tzinfo=timezone.utc)
commit_to_compare_dt = datetime.fromisoformat("2025-11-11 08:18:48 -0800")
else:
is_pr_from_fork = os.getenv("IS_PR_FROM_FORK", "false") == "true"
_log(f" (implicit) is_pr_from_fork : {is_pr_from_fork}")
Expand All @@ -229,6 +240,8 @@ def retrieve_bucket_info(
else:
if external_repo == "":
bucket = "therock-ci-artifacts"
if curr_commit_dt and curr_commit_dt <= commit_to_compare_dt:
bucket = "therock-artifacts"
elif (
repo_name == "therock-releases-internal"
and owner == "ROCm"
Expand All @@ -237,6 +250,8 @@ def retrieve_bucket_info(
bucket = "therock-artifacts-internal"
else:
bucket = "therock-ci-artifacts-external"
if curr_commit_dt and curr_commit_dt <= commit_to_compare_dt:
bucket = "therock-artifacts-external"

_log("Retrieved bucket info:")
_log(f" external_repo: {external_repo}")
Expand Down
25 changes: 24 additions & 1 deletion build_tools/github_actions/tests/github_actions_utils_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,9 +46,20 @@ def test_gha_query_workflow_run_information(self):
"GITHUB_TOKEN not set, skipping test that requires GitHub API access",
)
def test_retrieve_bucket_info(self):
# TODO(geomin12): work on pulling these run IDs more dynamically
# https://github.com/ROCm/TheRock/actions/runs/18022609292?pr=1597
external_repo, bucket = retrieve_bucket_info("ROCm/TheRock", "18022609292")
self.assertEqual(external_repo, "")
self.assertEqual(bucket, "therock-artifacts")

@unittest.skipUnless(
os.getenv("GITHUB_TOKEN"),
"GITHUB_TOKEN not set, skipping test that requires GitHub API access",
)
def test_retrieve_newer_bucket_info(self):
# https://github.com/ROCm/TheRock/actions/runs/19680190301
external_repo, bucket = retrieve_bucket_info("ROCm/TheRock", "19680190301")
Copy link
Contributor

@kiran-thumma kiran-thumma Dec 2, 2025

Choose a reason for hiding this comment

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

Can you explain why are we adding a hardcoded runid. Can you clarify ?

Copy link
Contributor Author

@geomin12 geomin12 Dec 2, 2025

Choose a reason for hiding this comment

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

those are unit tests :)

Copy link
Contributor

@kiran-thumma kiran-thumma Dec 2, 2025

Choose a reason for hiding this comment

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

So for Unit tests do we have to hardcode the runid of an action ?

Copy link
Contributor Author

@geomin12 geomin12 Dec 2, 2025

Choose a reason for hiding this comment

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

yes, in this case, we are pulling GH data with "retrieve_bucket_info" and running asserts to ensure it works

For the changes I made, we need to check whether it's an old bucket or new bucket, and this is done via run ID

Copy link
Member

@ScottTodd ScottTodd Dec 2, 2025

Choose a reason for hiding this comment

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

We could also mock the network requests, in which case the run IDs could be arbitrary numbers. I started on that in https://github.com/ScottTodd/TheRock/tree/gha-test-mock.

I do like at least having the ability to test against the real github API with actual data though.

Copy link
Contributor

@kiran-thumma kiran-thumma Dec 2, 2025

Choose a reason for hiding this comment

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

Yes, we have to use something dynamic instead of hardcode run IDs. Please check if there any methods to pull the run ID dynamically.

Copy link
Contributor Author

@geomin12 geomin12 Dec 2, 2025

Choose a reason for hiding this comment

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

For pulling run IDs dynamically, it's fairly easy to do this for new jobs. for old jobs, this would be quite an effort on GH APIs and for unit tests.

Since these run IDs are known and usable numbers, I'm going to keep these static for unit testing

Copy link
Contributor

@kiran-thumma kiran-thumma Dec 3, 2025

Choose a reason for hiding this comment

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

Can we add a TODO comment on that.

self.assertEqual(external_repo, "")
self.assertEqual(bucket, "therock-ci-artifacts")

@unittest.skipUnless(
Expand All @@ -59,7 +70,7 @@ def test_retrieve_bucket_info_from_fork(self):
# https://github.com/ROCm/TheRock/actions/runs/18023442478?pr=1596
Copy link
Contributor

@kiran-thumma kiran-thumma Dec 2, 2025

Choose a reason for hiding this comment

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

If we can remove this commented lines and add some docs as comments for This function will be helpful to understand.

Copy link
Contributor Author

@geomin12 geomin12 Dec 2, 2025

Choose a reason for hiding this comment

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

there are docs in retrieve_bucket_info, while this function is a unit test. let me know if more clarification is needed

Copy link
Contributor

@kiran-thumma kiran-thumma Dec 2, 2025

Choose a reason for hiding this comment

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

Can you add comments where is the retreive_bucket_info. That might help who are looking at the code.

Copy link
Contributor Author

@geomin12 geomin12 Dec 2, 2025

Choose a reason for hiding this comment

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

Since this is a unit test suite for github_action_utils and it is testing various functions from github_action_utils, I hope that is clear enough? similar to gha_query_workflow_run_information function

although if still confusing, i can add a comment if needed

external_repo, bucket = retrieve_bucket_info("ROCm/TheRock", "18023442478")
self.assertEqual(external_repo, "ROCm-TheRock/")
self.assertEqual(bucket, "therock-ci-artifacts-external")
self.assertEqual(bucket, "therock-artifacts-external")
Copy link
Contributor

@kiran-thumma kiran-thumma Dec 2, 2025

Choose a reason for hiding this comment

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

Can we add this bucket dynamically by using other variables instead of hardcode the bucket name.

Copy link
Contributor Author

@geomin12 geomin12 Dec 2, 2025

Choose a reason for hiding this comment

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

the "dynamic retrieval" is actually the bucket variable. the "therock-artifacts-external" string is what the variable is comparing too for unit tests correctness!


@unittest.skipUnless(
os.getenv("GITHUB_TOKEN"),
Expand All @@ -71,6 +82,18 @@ def test_retrieve_bucket_info_from_rocm_libraries(self):
"ROCm/rocm-libraries", "18020401326"
)
self.assertEqual(external_repo, "ROCm-rocm-libraries/")
self.assertEqual(bucket, "therock-artifacts-external")

@unittest.skipUnless(
os.getenv("GITHUB_TOKEN"),
"GITHUB_TOKEN not set, skipping test that requires GitHub API access",
)
def test_retrieve_newer_bucket_info_from_rocm_libraries(self):
Copy link
Contributor

@kiran-thumma kiran-thumma Dec 2, 2025

Choose a reason for hiding this comment

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

Same as above can you add some docs in comments explaining the function.

Copy link
Contributor Author

@geomin12 geomin12 Dec 2, 2025

Choose a reason for hiding this comment

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

there are docs in retrieve_bucket_info, while this function is a unit test. let me know if more clarification is needed

# https://github.com/ROCm/rocm-libraries/actions/runs/19784318631
external_repo, bucket = retrieve_bucket_info(
"ROCm/rocm-libraries", "19784318631"
)
self.assertEqual(external_repo, "ROCm-rocm-libraries/")
self.assertEqual(bucket, "therock-ci-artifacts-external")

@unittest.skipUnless(
Expand Down
Loading