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

[Feature Request][Pull request following] Support @AutoDefault for "required-key-but-not-required-value" behavior #15

Closed
divyekapoor opened this issue Oct 27, 2021 · 4 comments

Comments

@divyekapoor
Copy link
Contributor

Hi Brian,
Thanks for making a fantastic config library. We (at Pinterest) have been using it extensively to set up our configuration infrastructure for our streaming applications.
One common pattern we have seen is that we often set up configs in the following manner:

code.java
interface ConfigIface {
  @Config("c")
  String getConfigInternal();

  default String getConfig() {
     if ("auto".equalsIgnoreCase(getConfigInternal())) {
       return "some-value-that-we-set-in-just-1-location-instead-of-50-configs-that-we-can-evolve";
     }
    return getConfigInternal();
  }
}

config.properties:
my.friendly.prefix.c=auto

The reason we do it this way is so that client teams know that the config actually exists (and can be overridden by them) but

The proposal is supporting this via a new annotation: @AutoDefault to make code like this:

interface ConfigIface {
  @Config("c")
  @AutoDefault("some-value-that-we-set-in-just-1-location-instead-of-50-configs-that-we-can-evolve")
  String getConfig();
}

config.properties:
my.friendly.prefix.c=auto

The difference with @Default is that the key is actually required in the config (so the configs all look the same and it's super clear where the framework is setting a value and where a user is setting a value). This is so common that we'd like to upstream it into the code so that we don't have to keep doing this.

Would you be open to accepting a pull request that implements this functionality?
It's a short change (10s of lines of code) - PR to follow.

@brianm
Copy link
Owner

brianm commented Nov 2, 2021

My concern is having a hardcoded magic value, "auto" -- what do you think about my suggestion in the PR to make a change to @Default to specify that it should override certain values with the default value?

@brianm brianm pinned this issue Nov 2, 2021
@divyekapoor
Copy link
Contributor Author

Responded on the PR: #16 (comment)
Thanks for working with us!

@brianm
Copy link
Owner

brianm commented Nov 3, 2021

Btw, the way I solved this discoverability problem in the past is just commenting out the unused config value. This takes it out of startup validation (a very valuable forcing function), but it worked for us.

@brianm
Copy link
Owner

brianm commented Nov 9, 2021

Thank you, @sebastiananu for the PR! It should be merged, and released. I had to do a bunch of pokery-jiggery with maven and oss.r.o, but it looks like it went through in version 0.22, which is staged and should sync with central whenever it gets synced.

@brianm brianm closed this as completed Nov 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants