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

chore(unittests): if not defined set a default region in boto3_proxy.py #232

Closed
wants to merge 2 commits into from

Conversation

mmaeng
Copy link
Contributor

@mmaeng mmaeng commented Dec 6, 2022

Closes #219

Unit tests do not require AWS credentials but were still failing if no region was set. Set default region in boto3_proxy

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@mmaeng mmaeng changed the title fix(tests): if not defined set a default region in boto3_proxy.py fix(unittests): if not defined set a default region in boto3_proxy.py Dec 6, 2022
@mmaeng mmaeng changed the title fix(unittests): if not defined set a default region in boto3_proxy.py chore(unittests): if not defined set a default region in boto3_proxy.py Dec 6, 2022
@@ -14,7 +14,7 @@ def __init__(self, session: Session):


def _get_boto_session(
credentials: Optional[Credentials], region: Optional[str] = None
credentials: Optional[Credentials], region: Optional[str] = "us-east-1"
Copy link
Contributor

Choose a reason for hiding this comment

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

I like the default being something other than None.

Wondering though.... why not

if not credentials or not region:
  return None

Seems weird to get credentials not be None but then have no region?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the check for region being explicitly set to none and tests around it

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure we want to have this be the default but I'm still trying to think through it all. While I get we are fixing for this for testing purposes I'm afraid of what will happen under normal operations:

So I'll try my best to explain. For a resource we are going to call _get_boto_session with the credentials and provide no region. If this resource lives in us-west-2 are we resetting the session to be in us-east-1? Is the region picked up from the environment variables provided in the Lambda runtime?

caller_sess = _get_boto_session(event.requestData.callerCredentials)
provider_sess = _get_boto_session(event.requestData.providerCredentials)

Those values are a class defined by either None or:

@dataclass
class Credentials:
accessKeyId: str
secretAccessKey: str
sessionToken: str

That being said from what I can tell we should not be testing for region being none.

@mmaeng
Copy link
Contributor Author

mmaeng commented Dec 21, 2022

#239 will also resolve the unittest errors but without altering main program behavior.

@mmaeng mmaeng closed this Dec 21, 2022
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.

Unittests are requiring region to be set on the command line to pass
3 participants