chore: add deprecated flags to force early crash when used #165
Conversation
|
It looks really bad on us if we simply just crash. It is not a common practice, nor not backward compatible. I think we should treat it more seriously, and It is a hard problem, perhaps we can devise a runbook to change the flags. For this issue, we will take the hard job. I will create a doc for such scheme |
|
Ok, we will resume the task. |
samlaf
force-pushed
the
backward-compatible-flags
branch
from
52da8ab
to
4da472e
Compare
flags/eigendaflags/deprecated.go
Outdated
| &cli.BoolFlag{ | ||
| Name: DeprecatedWaitForFinalizationFlagName, | ||
| Usage: "Wait for blob finalization before returning from PutBlob.", | ||
| EnvVars: withDeprecatedEnvPrefix(envPrefix, "WAIT_FOR_FINALIZATION"), |
There was a problem hiding this comment.
WAIT_FOR_FINALIZATION isn't introduced before
flags/eigendaflags/deprecated.go
Outdated
| } | ||
|
|
||
| func withDeprecatedEnvPrefix(envPrefix, s string) []string { | ||
| return []string{envPrefix + "_" + s} |
There was a problem hiding this comment.
for many ENV, there is not list of string, can you change
There was a problem hiding this comment.
For example, SERVICE_MANAGER_ADDR, list notation is confusing
The MEMstore one is better
There was a problem hiding this comment.
Can you also change here, https://github.com/Layr-Labs/eigenda-proxy/blob/main/verify/cli.go#L44.
With the latest change, it looks like this
Not sure why it has to be a list
|
Besides those, I tested manually, everything work, nice |
didn't exist before, so shouldnt be deprecated - its a new flag
Recent flags refactor PR changed a bunch of flags. Added some those deprecated flags back, crashing the program if they are ever used to force users to change to the new flags.
I'm sure I missed a bunch of flags though given that the flags have been dispersed locally in a bunch of different modules. Would be a tough job to make sure to get all the deprecated flags... I propose we leave it at this. @bxue-l2 wdyt?