-
Notifications
You must be signed in to change notification settings - Fork 246
DRIVERS-3131: No explicitly provided properties with MONGODB-AWS #1847
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
base: master
Are you sure you want to change the base?
Conversation
cb6a6eb
to
30808eb
Compare
mongodb://<AccessKeyId>:<SecretAccessKey>@localhost/?authMechanism=MONGODB-AWS | ||
mongodb://localhost/?authMechanism=MONGODB-AWS | ||
``` | ||
|
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.
We should also update the other prose tests that are no longer relevant in this file (strike-through, or replace the title with *Removed*
per prose test guidelines)
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.
All the the prose tests are still valid and I updated the assume role test to note as well to pass --nouri
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.
I believe Case 1 above would fall into the category of "no longer relevant" after this set of changes, so I was suggesting the strike-through method. There is also a callout about URI encoding in L15-16 of this file that should probably be amended.
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.
I've added the Removed label to the titles.
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.
I don't think those are the tests we want to remove. In addition, the guidelines for removing tests say to either remove the whole test and replace the title with "Removed" OR strikethrough the entire content - just adding a "removed" label and keeping everything else in place might be confusing.
mongodb://<AccessKeyId>:<SecretAccessKey>@localhost/?authMechanism=MONGODB-AWS | ||
mongodb://localhost/?authMechanism=MONGODB-AWS | ||
``` | ||
|
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.
I believe Case 1 above would fall into the category of "no longer relevant" after this set of changes, so I was suggesting the strike-through method. There is also a callout about URI encoding in L15-16 of this file that should probably be amended.
Drivers MUST test the following scenarios: | ||
|
||
1. `Regular Credentials`: Auth via an `AWS_ACCESS_KEY_ID` and `AWS_SECRET_ACCESS_KEY` pair | ||
1. `Regular Credentials`: Auth via an `AWS_ACCESS_KEY_ID` and `AWS_SECRET_ACCESS_KEY` pair *Removed* |
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.
Don't we still want to test this with env vars?
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.
Yes, this is now fixed.
source/auth/tests/mongodb-aws.md
Outdated
Expect authentication to succeed and the custom credential provider was called. | ||
|
||
## Regular credentials | ||
## Regular credentials *Removed* |
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.
I don't think this is the right test to remove, #### Case 1: Credentials in URI Take Precedence
is the one that isn't relevant with the new changes.
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.
Brought back.
mongodb://<AccessKeyId>:<SecretAccessKey>@localhost/?authMechanism=MONGODB-AWS | ||
mongodb://localhost/?authMechanism=MONGODB-AWS | ||
``` | ||
|
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.
I don't think those are the tests we want to remove. In addition, the guidelines for removing tests say to either remove the whole test and replace the title with "Removed" OR strikethrough the entire content - just adding a "removed" label and keeping everything else in place might be confusing.
This has been fixed. |
Removes supplying credentials for MONGODB-AWS in the URI or client options in the auth spec and adds new tests.
Tools changes: mongodb-labs/drivers-evergreen-tools#691
Node implementation: mongodb/node-mongodb-native#4689
Please complete the following before merging:
clusters).