-
Notifications
You must be signed in to change notification settings - Fork 7k
[ci] fix postmerge tests that require credentials #57915
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.
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
Conversation
86cac62 to
9d3479e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request aims to simplify CI test setup by using awscli directly instead of installing boto3. The changes in ci/env/setup_credentials.py correctly replace the boto3 dependency with a subprocess call to awscli. The corresponding pip install commands are removed from the Buildkite YAML files. I've found a critical issue where one of the CI steps is changed to use a setup_credentials.sh script that is not part of this PR, which will likely break the build. I've also suggested an improvement to the exception handling in the Python script to make it more robust. Overall, the changes are in the right direction, but the missing script needs to be addressed.
| try: | ||
| client = boto3.client("secretsmanager", region_name="us-west-2") | ||
| ray_air_secrets = get_ray_air_secrets(client) | ||
| ray_air_secrets = get_ray_air_secrets() | ||
| except Exception as e: | ||
| print(f"Could not get Ray AIR secrets: {e}") | ||
| sys.exit(1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The exception handling here is very broad, catching Exception. While the previous code did this as well, the new implementation using subprocess can raise different types of exceptions like subprocess.CalledProcessError, FileNotFoundError (if aws isn't in PATH), json.JSONDecodeError, or KeyError. It would be more robust to handle these specific exceptions to provide more informative error messages and improve debuggability. Also, it's a good practice to print error messages to sys.stderr.
| try: | |
| client = boto3.client("secretsmanager", region_name="us-west-2") | |
| ray_air_secrets = get_ray_air_secrets(client) | |
| ray_air_secrets = get_ray_air_secrets() | |
| except Exception as e: | |
| print(f"Could not get Ray AIR secrets: {e}") | |
| sys.exit(1) | |
| try: | |
| ray_air_secrets = get_ray_air_secrets() | |
| except (subprocess.CalledProcessError, FileNotFoundError) as e: | |
| print(f"Error executing aws-cli command: {e}", file=sys.stderr) | |
| sys.exit(1) | |
| except (json.JSONDecodeError, KeyError) as e: | |
| print(f"Error parsing secrets from aws-cli output: {e}", file=sys.stderr) | |
| sys.exit(1) |
9d3479e to
1f31bdd
Compare
use awscli directly; stop installing extra dependencies Signed-off-by: Lonnie Liu <[email protected]>
1f31bdd to
729963d
Compare
| "--secret-id", | ||
| AWS_AIR_SECRETS_ARN, | ||
| ] | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Missing AWS CLI Causes Credential Setup Failure
The setup_credentials.py script now calls the aws CLI directly, but the CI configuration removed the awscli installation. This prevents the aws command from being found, causing the credential setup to fail and impacting tests that rely on these secrets.
Additional Locations (1)
use awscli directly; stop installing extra dependencies Signed-off-by: Lonnie Liu <[email protected]> Signed-off-by: Kamil Kaczmarek <[email protected]>
use awscli directly; stop installing extra dependencies Signed-off-by: Lonnie Liu <[email protected]> Signed-off-by: xgui <[email protected]>
use awscli directly; stop installing extra dependencies Signed-off-by: Lonnie Liu <[email protected]> Signed-off-by: elliot-barn <[email protected]>
use awscli directly; stop installing extra dependencies Signed-off-by: Lonnie Liu <[email protected]>
use awscli directly; stop installing extra dependencies Signed-off-by: Lonnie Liu <[email protected]> Signed-off-by: Aydin Abiar <[email protected]>
use awscli directly; stop installing extra dependencies