Skip to content
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

SEC-19137 Add additional get_aws_credentials tests to spark-run #3919

Merged
merged 4 commits into from
Jul 18, 2024

Conversation

choww
Copy link
Contributor

@choww choww commented Jul 11, 2024

❗️Must be merged after #3901

Add tests to spark-run to make sure get_aws_credentials() from service_configuration_lib is invoked properly by the various flags passed to paasta spark-run:

  • --service should not overwrite --aws-profile
  • use --service if --aws-profile is absent
  • in absence of a valid --service and --aws-profile, use --aws-profile=default

@mock.patch("service_configuration_lib.spark_config.Session", autospec=True)
def test_get_aws_credentials_session(mock_boto3_session, mock_use_aws_profile):
# prioritize session over `profile_name` if both are provided
from service_configuration_lib.spark_config import get_aws_credentials
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason to not have imports at the top? LGTM otherwise

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved all the import statements!

Copy link
Contributor

@CaptainSame CaptainSame left a comment

Choose a reason for hiding this comment

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

Nit

tests/cli/test_cmds_spark_run.py Outdated Show resolved Hide resolved
@choww choww merged commit 6b8bca6 into master Jul 18, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants