Skip to content

Update HELLO cmd #11037

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

Open
wants to merge 6 commits into
base: unstable
Choose a base branch
from
Open

Conversation

KarthikSubbarao
Copy link
Contributor

@KarthikSubbarao KarthikSubbarao commented Jul 26, 2022

This PR updates the hello command so that all validation is done before either the name is applied or authentication is checked. Additionally, only one authentication or name is ever applied, with the following rules:

  • When a chain of AUTH sub commands are provided, we will only attempt authenticating with the credentials of the final AUTH sub command.
  • When a chain of SETNAME sub commands are provided, we will only attempt setting the client name with the name of the final SETNAME sub command.
  • If the SETNAME and AUTH sub commands are provided, and if the client name is invalid, we will return an error and will not attempt authenticating. Similarly, if authentication fails, we will return an error and will not attempt to set the client name.

What is the motivation for this change?
This change came up while discussing the design for #10709, since it becomes awkward to yield multiple times to handle authentication. This change simplifies the code so only one callback ever needs to be triggered.

Is this a breaking change?
In a sense it is. All previously valid commands will continue to be valid and return the same result. A valid command is a command that contains only valid username password combinations and valid setnames. This change may behave differently when an error is thrown. There are a couple of new exemplar modes:

  • HELLO 2 SETNAME FOO AUTH INVALID PASSWORD: This command would previously set the client name to foo, before throwing an error about an invalid password. It will no longer set the client name to foo and still error the same way.
  • HELLO 2 AUTH INVALID PASSWORD AUTH VALID PASSWORD: This command would previously fail with an error about an invalid password, since the first password is invalid. It will now succeed, since the last authentication is valid.

@madolson madolson added the breaking-change This change can potentially break existing application label Jul 26, 2022
@oranagra oranagra added this to the Next major backlog milestone Jul 26, 2022
@CLAassistant
Copy link

CLAassistant commented Mar 24, 2024

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
0 out of 3 committers have signed the CLA.

❌ Karthik Subbarao
❌ madolson
❌ KarthikSubbarao


Karthik Subbarao seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change This change can potentially break existing application
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants