Skip to content

Mention one of rediss:// and valkeys:// in error message, not both#373

Merged
zuiderkwast merged 2 commits into
valkey-io:unstablefrom
karthyuom:scheme-refactor
Apr 25, 2024
Merged

Mention one of rediss:// and valkeys:// in error message, not both#373
zuiderkwast merged 2 commits into
valkey-io:unstablefrom
karthyuom:scheme-refactor

Conversation

@karthyuom

@karthyuom karthyuom commented Apr 25, 2024 •
edited by zuiderkwast
Loading

Copy link
Copy Markdown
Contributor

When using a TLS scheme for valkey-cli and benchmark-cli compiled without TLS, make the error message only mention the scheme used.

Before:

"valkeys:// and rediss:// are only supported when valkey-cli is compiled with OpenSSL"

After, depending on which scheme the user tried to use:

"valkeys:// is only supported when valkey-cli is compiled with OpenSSL"
"rediss:// is only supported when valkey-cli is compiled with OpenSSL"

Follow up of #199.

@karthyuom
Refactor scheme specific changes as part of the re-branding.
8541ca3 
Signed-off-by: karthyuom <karthyuom@gmail.com>
@karthyuom

karthyuom commented Apr 25, 2024

Copy link
Copy Markdown
Contributor Author

@madolson @zuiderkwast could you please review the above when you get a chance. thanks.

zuiderkwast
zuiderkwast reviewed Apr 25, 2024
View reviewed changes
Comment thread src/cli_common.c
Comment thread src/cli_common.c Outdated
Comment thread src/cli_common.c
@karthyuom
Address review comments and fix build error.
31a5306 
Signed-off-by: karthyuom <karthyuom@gmail.com>
zuiderkwast
zuiderkwast approved these changes Apr 25, 2024
View reviewed changes

@zuiderkwast zuiderkwast left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks!

@zuiderkwast zuiderkwast changed the title Refactor scheme specific changes as part of the re-branding. Only mention used scheme (rediss:// or valkey://) in error message, not both Apr 25, 2024
@zuiderkwast zuiderkwast changed the title Only mention used scheme (rediss:// or valkey://) in error message, not both Only mention used scheme (rediss:// or valkeys://) in error message, not both Apr 25, 2024
@zuiderkwast zuiderkwast changed the title Only mention used scheme (rediss:// or valkeys://) in error message, not both Mention one of rediss:// and valkeys:// in error message, not both Apr 25, 2024
@codecov

codecov Bot commented Apr 25, 2024

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 60.00000% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 68.39%. Comparing base (d09a59c) to head (31a5306).
Report is 7 commits behind head on unstable.

Additional details and impacted files 
@@             Coverage Diff              @@
##           unstable     #373      +/-   ##
============================================
+ Coverage     68.37%   68.39%   +0.02%     
============================================
  Files           108      108              
  Lines         61562    61563       +1     
============================================
+ Hits          42091    42107      +16     
+ Misses        19471    19456      -15
Files Coverage Δ
src/cli_common.c 60.62% <60.00%> (-0.39%) ⬇️

... and 9 files with indirect coverage changes

@zuiderkwast zuiderkwast merged commit 9eaefc7 into valkey-io:unstable Apr 25, 2024
@roshkhatri

roshkhatri commented Jan 30, 2026

Copy link
Copy Markdown
Member

@zuiderkwast I think we need to backport this to 7.2 as well?

@github-project-automation github-project-automation Bot moved this to To be backported in Valkey 7.2 Jan 30, 2026
@roshkhatri roshkhatri removed this from Valkey 7.2 Jan 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zuiderkwast zuiderkwast zuiderkwast approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@karthyuom @roshkhatri @zuiderkwast