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

Set LibraryVersion in ConfigurationOptions #2533

Open
shacharPash opened this issue Aug 29, 2023 · 9 comments · May be fixed by #2547
Open

Set LibraryVersion in ConfigurationOptions #2533

shacharPash opened this issue Aug 29, 2023 · 9 comments · May be fixed by #2547

Comments

@shacharPash
Copy link
Contributor

I saw that now we can set the LibraryName in the ConfigurationOptions, what about LibraryVersion?
can we add LibraryVersion as well like here?

@shacharPash shacharPash changed the title LibraryVersion in ConfigurationOptions Set LibraryVersion in ConfigurationOptions Aug 29, 2023
@NickCraver
Copy link
Collaborator

What is the use case for changing it? The library name being modifiable to add more per-wrapping-application info was the intent with making it changeable, but the version is what we compile here and has a lot of value on the backend.

@shacharPash
Copy link
Contributor Author

shacharPash commented Aug 30, 2023

@NickCraver In NRedisStack I want to send the lib-name and lib-ver automatically (unless otherwise specified) when the client creates a connection, and I can easily do this with ConfigurationOptions if I can also set the LibraryVersion.

@NickCraver
Copy link
Collaborator

@shacharPash I think that makes sense, it's not how I think about it as seeing which versions are connecting and a server making decisions about that. For example, if I were seeing if all my clients were capable of X, this hides that ability since the library overriding these values could be anything, and using any version of SE.Redis underneath, making assessing what versions people are actually connecting with impossible.

Thoughts on the downsides there?

@shacharPash
Copy link
Contributor Author

Thanks for your consideration,
In my case I need to be able to send the lib-name&version of NRedisStack.
I don't see a particular downside here, for example, I thought of sending something like this:

LibraryName: NRedisStack (SE.Redis)
LibraryVersion: 0.9.1 (2.68.23423)

then the name and version of SE.Redis and NRedisStack will be written.

@mgravell
Copy link
Collaborator

mgravell commented Sep 3, 2023

I'm broadly OK with enabling this; if we don't folks will just bypass via Execute, so... maybe we make it easy to do well, perhaps with placeholder tokens for the default values?

@shacharPash
Copy link
Contributor Author

Hey @mgravell @NickCraver ,
Do you want me to send a PR or are you working on it?

@shacharPash
Copy link
Contributor Author

Hi, just updating that in the SETINFO command it is not possible to use ( and ), therefore if we want to write both SE.Redis and the library that uses it, we will have to write it in a different way from this:

LibraryName: NRedisStack (SE.Redis)
LibraryVersion: 0.9.1 (2.68.23423)

@NickCraver
Copy link
Collaborator

@shacharPash Sorry didn't see your latest there - I wanted to get RESP3 in before doing this as that whole area conflicts, but it's in now - was taking a look this morning. My plan is to separate these 2, and just outright allow overwriting LibraryVersion, and adding tokens to both name and version in a follow-up PR. Taking a peek at it now.

@NickCraver NickCraver linked a pull request Sep 10, 2023 that will close this issue
@NickCraver
Copy link
Collaborator

Working this in #2547

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants