[ci] refactor auth out of copy_files

[andrew-anyscale]

For the Wanda version of building each Ray Image, we want to selectively upload to DockerHub only in certain postsubmit cases. Rather than calling on 'buildkite:copy_files --destination docker_login' separately, this refactor pushes the auth setup to a central library that can be called on certain conditions.

Without this, logic would need to be pushed into the Rayci step itself, since current login is gated on a per-Pipeline-ID basis.

Topic: auth-refactor
Relative: upload-guards
Signed-off-by: andrew andrew@anyscale.com

[andrew-anyscale]

Reviews in this chain:
└#60612 [ci] add upload guards and multi-platform support to push_ray_image
　└#60614 [ci] refactor auth out of copy_files
　　└#60455 [ci]: port Buildkite Ray Images to Wanda
　　　├#59937 [ci] Add wanda anyscale image builds for release tests
　　　└#60539 Consolidate RAY_VERSION to use rayci.env as single source of truth

[andrew-anyscale]

#	head	base	diff	date	summary
0	68188ccb	2195731c	diff	Jan 30 10:19 AM	6 files changed, 103 insertions(+), 48 deletions(-)
1	7452f4cf	2195731c	diff	Jan 30 10:46 AM	1 file changed, 23 insertions(+), 8 deletions(-)
2	54061a8c	e8e58b5f	rebase	Jan 30 10:58 AM	0 files changed
3	9fc08d89	26fe1233	diff	Jan 30 10:59 AM	0 files changed

[gemini-code-assist]

Code Review

This pull request refactors authentication logic into a new centralized library, ci/ray_ci/rayci_auth.py, which is a great improvement for code organization and reusability. The new implementation for Docker Hub login also enhances security by using --password-stdin, preventing password exposure in the process list. My review focuses on the new rayci_auth.py file, offering suggestions to improve its robustness, maintainability, and error handling.

[gemini-code-assist]

The _retry decorator can be improved. As this logic is now in a shared library, making it more robust is important.

• Docstring Mismatch: The docstring claims "exponential backoff", but the implementation uses a fixed time.sleep(5).
• Exiting the process: The function calls sys.exit(1) on failure. In a library module, it's better to raise an exception to allow the caller to handle the error. This makes the function more reusable.

Suggested change

	def _retry(f):
	"""Retry decorator for API calls with exponential backoff on 5xx errors."""
	def inner():
	resp = None
	for _ in range(5):
	resp = f()
	print("Getting Presigned URL, status_code", resp.status_code)
	if resp.status_code >= 500:
	print("errored, retrying...")
	print(resp.text)
	time.sleep(5)
	else:
	return resp
	if resp is None or resp.status_code >= 500:
	print("still errored after many retries")
	sys.exit(1)
	return inner
	def _retry(f):
	"""Retry decorator for API calls with a fixed-interval retry on 5xx errors."""
	def inner():
	resp = None
	for _ in range(5):
	resp = f()
	print("Getting Presigned URL, status_code", resp.status_code)
	if resp.status_code >= 500:
	print("errored, retrying...")
	print(resp.text)
	time.sleep(5)
	else:
	return resp
	if resp is None or resp.status_code >= 500:
	print("still errored after many retries")
	raise RuntimeError("Failed to get a valid response from RayCI API after multiple retries.")
	return inner

[gemini-code-assist]

The AWS region and service are hardcoded here. To improve maintainability and configurability, consider defining them as module-level constants, similar to RAYCI_API_HOST. You would then define RAYCI_API_REGION and RAYCI_API_SERVICE at the top of the file.

Suggested change

	aws_region="us-west-2",
	aws_service="execute-api",
	aws_region=RAYCI_API_REGION,
	aws_service=RAYCI_API_SERVICE,

[gemini-code-assist]

Direct access to os.environ["BUILDKITE_JOB_ID"] will raise a KeyError if the environment variable is not set. It's better practice to use .get() and raise a more informative ValueError if it's missing.

Suggested change

	resp = requests.get(
	RAYCI_API_URL,
	auth=auth,
	params={"job_id": os.environ["BUILDKITE_JOB_ID"]},
	)
	job_id = os.environ.get("BUILDKITE_JOB_ID")
	if not job_id:
	raise ValueError("BUILDKITE_JOB_ID environment variable must be set.")
	resp = requests.get(
	RAYCI_API_URL,
	auth=auth,
	params={"job_id": job_id},
	)

[gemini-code-assist]

This line can raise a KeyError or requests.exceptions.JSONDecodeError if the API response is not as expected. It's safer to wrap this in a try...except block to handle potential errors gracefully and provide a more informative error message.

    try:
        pwd = resp.json()["docker_password"]
    except (requests.exceptions.JSONDecodeError, KeyError):
        raise ValueError(f"Could not get docker password from API. Response: {resp.text}")

[cursor]

Cursor Bugbot has reviewed your changes and found 1 potential issue.

[cursor]

subprocess.check_call does not support input parameter

High Severity

The docker_hub_login function uses subprocess.check_call() with an input parameter, but check_call doesn't support input. This parameter is only available in subprocess.run(). When called, this will raise a TypeError because the input kwarg gets passed to Popen, which doesn't accept it. The function needs to use subprocess.run(..., check=True) instead.