Skip to content

Conversation

@blink1073
Copy link
Member

@blink1073 blink1073 commented May 9, 2023

The script can be called as follows:

cd .evergreen/auth_aws
. ./activate-authawsvenv.sh
python aws_tester.py ec2  # ecs, assume-role, regular, web-identity

Tested with the Python driver with all 5 variants here: https://spruce.mongodb.com/task/mongo_python_driver_aws_auth_test__platform~ubuntu_18.04_python_version~3.7_aws_auth_test_6.0_patch_873032660bf22d09cbf0013f6b077196d0b95f40_645aaaa63e8e86cbdcbd8933_23_05_09_20_18_56/logs?execution=1&sortBy=STATUS&sortDir=ASC

Linked PR: mongodb/mongo-python-driver#1220

@blink1073 blink1073 requested a review from alcaeus May 9, 2023 21:38
@blink1073
Copy link
Member Author

cc @dariakp

@blink1073 blink1073 changed the title DRIVERS-2328 Use Python and Mongo Shell for AWS Scripts DRIVERS-2328 Use Python and Mongo Shell for AWS Testing May 9, 2023
@durran
Copy link
Member

durran commented May 10, 2023

@blink1073 what are the proposed driver changes supposed to look like? I did a commit to run an Evergreen patch on the Node driver against your fork/branch to see what this looked like and I get some AWS tests passing, but some failing as well.

My driver changes: mongodb/node-mongodb-native@57c3f65

My patch: https://spruce.mongodb.com/version/645bf0c856234354a5daaec9/tasks?sorts=STATUS%3AASC%3BBASE_STATUS%3ADESC

@blink1073
Copy link
Member Author

Here's my patch:

mongodb/mongo-python-driver@mongodb:mongo-python-driver:master...blink1073:mongo-python-driver:PYTHON-3692

In the ones that are passing in your patch, I see the expected "Creating user" output in the log.

Perhaps set each command to use "bash" and add a "set -ex" so we can debug?

@ShaneHarvey
Copy link
Contributor

@markbenvenuto would you like to merge any of this work back into the server test suite so that we can keep parity?

@alcaeus
Copy link
Contributor

alcaeus commented May 11, 2023

I tested this using the .NET driver (I used that for all my testing as PHP does not implement these tests itself), and the only test that failed was the ecs test. However, the .NET driver has some special logic there to move the test file around, so I'd attribute the failure to my setup rather than assume it was the fault of these changes. I'll wait for @durran to run a new patch with debug output.

@durran
Copy link
Member

durran commented May 11, 2023

I cleaned up the patch to just test latest and added debug, only passing 3 of the 8. https://spruce.mongodb.com/version/645d33893627e020f687a8a5/tasks?sorts=STATUS%3AASC%3BBASE_STATUS%3ADESC

I haven't dug into the cause as of yet, but my changes look consistent with the Python changes. This is the latest diff: mongodb/node-mongodb-native@9584639

But I assume it's something in our setup, so I won't hold this PR up.

@blink1073
Copy link
Member Author

You need to add shell: bash whenever you are calling . ./activate-authawsvenv.sh.

@durran durran self-requested a review May 11, 2023 19:54
@blink1073
Copy link
Member Author

@durran you need to use export PROJECT_DIRECTORY="${ECS_SRC_DIR}";

Copy link
Member

@durran durran left a comment

Choose a reason for hiding this comment

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

From a functional standpoint these work on the Node branch now. I'll let someone else review the Python code itself.

Copy link
Contributor

@alcaeus alcaeus left a comment

Choose a reason for hiding this comment

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

Changes LGTM as well. I ran into failures when testing this with the C# driver but I assume it's connected to the setup (which I'm not entirely familiar with) rather than the tests itself.

What I didn't test was whether this will have a breaking impact on drivers, i.e. do drivers have to modify their evergreen config as soon as we merge this, or is this something they can opt into?

@blink1073
Copy link
Member Author

No changes are necessary, it is opt-in. I ran an EG patch using this branch but with no other changes to the config, and it 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.

4 participants