-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
fix: bug around using temporary credentials in event bridge integration test. #1829
Conversation
def _create_cred_file(self, profile, access_key, secret_key): | ||
cred_file_content = "[default]\naws_access_key_id = {1}\naws_secret_access_key = {2}\n" | ||
def _create_cred_file(self, profile, access_key, secret_key, session_token=None): | ||
cred_file_content = self._create_cred_profile("default", access_key, secret_key, session_token) |
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.
Doesn't this need to be [default]
?
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.
O... this is moved down to the helper method below.
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.
That is taken care of in the method line 133. It is [{profile_name}]
aws_access_key_id = {access_key} | ||
aws_secret_access_key = {secret_key} | ||
""" | ||
cred_profile_content += f"aws_session_token={session_token}\n" |
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.
Why not have this in the cred_profile_content
above?
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.
Ohh that is a miss, there is supposed to be an if in there to add this only if session_token is passed. Lost somewhere in merging. I'll fix it.
Issue #, if available:
Why is this change necessary?
Event bridge integration tests do no respect session token to run secure tests. This code change fixes that.
How does it address the issue?
Using session token if present.
What side effects does this change have?
Checklist:
make pr
passesBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.