Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat: add
do
command to update the authentication plugin of MySQL users #1097feat: add
do
command to update the authentication plugin of MySQL users #1097Changes from 3 commits
f9fb3bc
9ee515d
9735201
a17a0d6
cb1d1fa
216d0fd
49f850e
d32b8f8
735a6b1
6c927ce
eb3609e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
What is the command that should be run for a vanilla installation?
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 two commands necessary for a vanilla installation in the troubleshooting section as well
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.
This part about automatically loading password from config should be removed. In particular, with the current implementation, a warning will be printed even after user inputs a password.
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.
Regarding this,
The warnings will only be printed if the password entered by a user does not match the conventional format that tutor expects or if the value in config of the conventional key does not match the one entered by the user. This is just done to make sure no one accidentally updates the password of their mysql users and is left wondering why their platform doesn't work now.
There is also an option of using the command non-interactively.
Do you still think we should remove this? I understand its not intuitive at all but I added it just in case someone doesn't accidentally update their passwords.
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.
Yeah, I think we should remove this, as it will cause more confusion than resolve anything. If the password is incorrect, the command will fail, right? (which is good)
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.
Unfortunately, it wont fail. Instead, mysql will just update the password of the user to the incorrectly entered password and then the platform will be unable to connect because the password has been updated. This was my main motivation for adding these checks.
It only throws a warning if the username is incorrect, which makes sense cause then the user wouldn't exist anyway.
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.
OK, I understand. Here's what I suggest:
prompt=True
in the password option definition.In other words, the password is automatically detected if it matches a known username. Optionally, we can build
known_mysql_credentials
from a new filter.Change the documentation to tell users to run:
Would that work?
Sorry about the back and forth on this feature. I'm aware that this PR has been open for 6+ months, and I'm eager to merge it.
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 like this idea as well.
Since this is a one time command, lets not go this route. We can hardcode the values and if someone is using a custom plugin, they can just use the username password option.
I've updated the rest of the code and docs as per the method you've mentioned here.
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 think the following will be more robust:
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.
This certainly looks better. I've updated the original one to this.
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.
Let's not adopt the habit of documenting the functions at the top of the file. Can you please move the function short descriptions to the function docstrings? (in triple quotes)
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 did it as @DawoudSheraz requested me to add file docstrings. But, I guess we're not following that convention for now. So, I've removed the file docstring in favor of function docstrings.