Skip to content

Conversation

@blink1073
Copy link
Member

No description provided.

client.get_database().test.find_one()
self.assertNotEqual(auth.get_cached_credentials(), None)

def test_environment_variables_ignored(self):
Copy link
Member

Choose a reason for hiding this comment

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

Is this behavior outlined in the spec? Shouldn't the env vars take precedence even if we already have a previous cached credential?

Copy link
Member

Choose a reason for hiding this comment

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

Discussed in person, this behavior happens because we rely on aws for the credential lookup. The code is like:

if cached_creds:
    return cached_creds
creds = get_aws_creds()
if should_cache(creds):
    cached_creds = creds
return creds

So if the app is always using env vars then there will never be a cached credential and dynamic env var changes will propagate immediately. However, if the app is using temp creds then later tries to switch to dynamic env var creds, then changes won't propagate until the cached creds are cleared.

@blink1073 blink1073 requested a review from ShaneHarvey November 4, 2022 14:48
@blink1073
Copy link
Member Author

We updated the spec to clarify that changes to the environment variables should be isolated in case the tests are run in parallel.

@ShaneHarvey
Copy link
Member

We updated the spec to clarify that changes to the environment variables should be isolated in case the tests are run in parallel.

That's fine but I just want to confirm that mock.patch doesn't actually add thread safety to these tests.

@blink1073
Copy link
Member Author

That's fine but I just want to confirm that mock.patch doesn't actually add thread safety to these tests.

Confirmed. Based on the spec that means we should use unit tests instead, which means those tests will need to go in pymongo_auth_aws.

@ShaneHarvey
Copy link
Member

ShaneHarvey commented Nov 4, 2022

I'm confused, I don't think we need to do anything regarding the parallel test issue because we don't run the python tests in parallel. What kind of change are you thinking?

@blink1073
Copy link
Member Author

I'm confused, I don't think we need to do anything regarding the parallel test issue because we don't run the python tests in parallel.

Yeah that's fair, I'll update the spec a bit to clarify.

@blink1073 blink1073 merged commit ff94b0e into mongodb:master Nov 4, 2022
@blink1073 blink1073 deleted the PYTHON-3501 branch November 4, 2022 19:25
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.

2 participants