make backward compatible #163
Conversation
| @@ -28,7 +28,7 @@ func withFlagPrefix(s string) string { | |||
| } | |||
|
|
|||
| func withEnvPrefix(envPrefix, s string) []string { | |||
| return []string{envPrefix + "_EIGENDA_" + s} | |||
| return []string{envPrefix + "_EIGENDA_CLIENT_" + s, envPrefix + "_" + s} | |||
There was a problem hiding this comment.
Add comment explaining that second one is just for backward compatibility. We should also prob add a deprecation notice? Maybe in the category string?
Also are we sure this makes everything backward compatible? Did some of them not have just EIGeNDA prefix for eg? Also flags themselves can’t have multiple options so those won’t be backward compatible unless we add more flags (which I am against… but wdyt @jianoaix )
There was a problem hiding this comment.
I went through flags one by one, they are backward compatible, except for memstore. But since memstore is only used in testnet, so it is fine to break it.
There was a problem hiding this comment.
Ah true for memstore. But I’m pretty sure the eigenDA flags were —eigenda-XXx before whereas now they are —eigenda.XXx
There was a problem hiding this comment.
Adding a deprecation note is good. We could put it in the readme part, https://github.com/Layr-Labs/eigenda-proxy?tab=readme-ov-file#configuration-options
There was a problem hiding this comment.
so in the readme, we specify the canonical format of the env variable, but also note we embrace backward compatibility
There was a problem hiding this comment.
SG. Also +1 to add deprecation notice. Not sure if the flag framework supports auto notice; if not, maybe we can emit logs if the old flag is used to start the binary.
There was a problem hiding this comment.
I think in Python they have a pattern (actually even library like https://pypi.org/project/deprecation/) using a @deprecated decorator to annotate the things you want to deprecate. We may at least 1) document, e.g. specifying when it's deprecated, and when it'll be removed with a release version; 2) log the attempts to use deprecated API/flags.
There was a problem hiding this comment.
@bxue-l2 thought about this more and I think I agree with @jianoaix now. Doing multiple env_vars on the same flag is not the way to go, because that makes it very hard to give deprecation notices.
If we create new flags instead, then we can use flag actions to print a deprecation notice on the old flags. And when we release 2.0.0 we can remove the deprecated flags.
There was a problem hiding this comment.
I agree. I can close this PR. There 2 solutions I can see is to
Plan A: revert the flags PRs
Plan B:
- hand pick the old env variables (i.e. those that have new names)
- keep the new names, create a sections about the list of flags for old names
There was a problem hiding this comment.
Agree. I think Plan B works best, because the flags PR contains a lot of other flag refactors... will be hard to dismantle properly. I'm thinking we create a new category EIGENDA_DEPRECATED_FLAGS or something (so that these flags don't clutter the --help output), and add all the backward compatible deprecated flags there. Should be in its own file probably, such that when we bump to 2.0.0 we can just delete that file to easily remove all of them.
Make env backward compatible.
Method: Adding multiple EnvVars
The urfave doc shows, https://cli.urfave.org/v2/examples/flags/#values-from-the-environment
If EnvVars contains more than one string, the first environment variable that resolves is used.
If this approach looks fine, I will go ahead to change the template env, https://github.com/Layr-Labs/eigenda-proxy/blob/main/.env.example.holesky
Also add a new column to he configuration option in the readme page. but @samlaf , do you want to take the rest?
sample