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

implement dbshell #20

Merged
merged 1 commit into from
May 8, 2024
Merged

implement dbshell #20

merged 1 commit into from
May 8, 2024

Conversation

WaVEV
Copy link
Collaborator

@WaVEV WaVEV commented May 8, 2024

Implementation of MongoDB connection setup method in DatabaseClient class.

Description:
This PR introduces the settings_to_cmd_args_env method in the DatabaseClient class.

Limitations:

  1. how do we handle options like awsIamSessionToken.
  2. Currently, password hiding under environment variables is not supported.

Feedback and contributions are welcomed to address these limitations and refine the implementation.

  • Where do I have to put the unit test, I already wrote it, but I don't know where those have to be.. will they be merged into Django test? solved. PR open
  • Hide password if an exception is raised.

@WaVEV WaVEV requested review from timgraham and Jibola May 8, 2024 03:20
@WaVEV WaVEV mentioned this pull request May 8, 2024
2 tasks
@timgraham timgraham changed the title Mongodb dbshell. implement dbshell May 8, 2024
@timgraham
Copy link
Collaborator

In looking at https://www.mongodb.com/docs/mongodb-shell/write-scripts/, I don't see any advantage to using dbshell to execute JavaScript files. The JavaScript has to make its own connection to MongoDB... there's no way to use Django's connection. So let's omit that functionality. My guess is that most Python developers using Django will want to write Python scripts, not JavaScript!

For the commit message, use:

implement dbshell

fixes #3

Copy link
Collaborator

@timgraham timgraham left a comment

Choose a reason for hiding this comment

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

Squash into a single commit for merge or else we can use the "squash and merge" green button.


@classmethod
def settings_to_cmd_args_env(cls, settings_dict, parameters):
raise NotImplementedError
def settings_to_cmd_args_env(cls, settings_dict, parameters): # noqa: ARG003 parameters argument is not being used
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we could shorten that a bit: # noqa: ARG003 parameters unused


host = settings_dict["HOST"]
port = settings_dict["PORT"]
dbname = settings_dict["NAME"] or "test"
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove "test" now that parameters are gone

@WaVEV WaVEV merged commit a48e325 into mongodb-labs:main May 8, 2024
3 checks passed
@WaVEV WaVEV deleted the dbshell branch May 8, 2024 19:19
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