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.

Sign up for GitHub

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

make backward compatible #163

Closed
wants to merge 1 commit into from
Closed

make backward compatible #163

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion flags/eigendaflags/cli.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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}
Copy link
Collaborator

Choose a reason for hiding this comment

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

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 )

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

@samlaf samlaf Oct 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Ah true for memstore. But I’m pretty sure the eigenDA flags were —eigenda-XXx before whereas now they are —eigenda.XXx

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

so in the readme, we specify the canonical format of the env variable, but also note we embrace backward compatibility

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@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.

Copy link
Collaborator Author

@bxue-l2 bxue-l2 Oct 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I agree. I can close this PR. There 2 solutions I can see is to
Plan A: revert the flags PRs
Plan B:

  1. hand pick the old env variables (i.e. those that have new names)
  2. keep the new names, create a sections about the list of flags for old names

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

}

// CLIFlags ... used for EigenDA client configuration
Expand Down
2 changes: 1 addition & 1 deletion store/precomputed_key/s3/cli.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ func CLIFlags(envPrefix, category string) []cli.Flag {
&cli.StringFlag{
Name: EndpointFlagName,
Usage: "endpoint for S3 storage",
EnvVars: withEnvPrefix(envPrefix, "S3_ENDPOINT"),
EnvVars: withEnvPrefix(envPrefix, "ENDPOINT"),
Category: category,
},
&cli.StringFlag{
Expand Down
2 changes: 1 addition & 1 deletion verify/cli.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,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}
}

// CLIFlags ... used for Verifier configuration
Expand Down
Loading