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

PT-1776: Adding in slave-user and slave-password for dsn table based … #425

Open
wants to merge 2 commits into
base: 3.0
Choose a base branch
from

Conversation

mbenshoof
Copy link

…recursion

@it-percona
Copy link
Contributor

it-percona commented Oct 25, 2019

CLA assistant check
All committers have signed the CLA.

Copy link
Collaborator

@svetasmirnova svetasmirnova left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution.

You modified the auto-generated code. It will be removed once we run the update-modules utility. Please put your changes into the MasterSlave package.

@svetasmirnova
Copy link
Collaborator

Another issue with this proposal is that it modifies DSN table entries without checking if they have user and password defined. This could cause issues when users have, say, 10 replicas with the same user/password pairs and two with different ones.

We also need a test case to accept this PR.

@svetasmirnova
Copy link
Collaborator

There is also similar PR at #381

… making them not overwrite what is specified in DSN table.
@mbenshoof
Copy link
Author

@svetasmirnova - Modified the logic to check for user/pass specified in DSN string. Only sets the CLI passed options if they are not set in the DSN table.

For the test case, I've been using dbdeployer for a quick primary/replica environment and setting different variations of DSN string in the table (with both user/pass, with only one, with neither) to validate it doesn't overwrite, but sets if missing

@svetasmirnova
Copy link
Collaborator

@mbenshoof

Thank you for the changes; I will review them in the new year.

For the test case, I've been using dbdeployer for a quick primary/replica environment and setting different variations of the DSN string in the table (with both user/pass, with only one, with neither) to validate it doesn't overwrite, but sets if missing

We need automatic test cases to run them at each release and ensure your changes are not overwritten by another contribution. We use a solution similar to MySQL Sandbox and dbdeployer for automatic test cases. Please check directory t and CONTRIBUTING.md.

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.

3 participants