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

env var only config #823

Merged
merged 5 commits into from
Oct 23, 2024
Merged

env var only config #823

merged 5 commits into from
Oct 23, 2024

Conversation

eschultink
Copy link
Member

Features

  • only allow some config as env vars

Change implications

  • dependencies added/changed? no
  • something important to note in future release notes? Yes
    • NOTE in CHANGELOG.md anything that will show up in terraform plan/apply that isn't
      obviously a no-op? if anyone is depending on overriding env vars with secrets/parameter store, this will break that
    • breaking changes? if in module/example that is NOT marked alpha, requires major version
      change

@eschultink eschultink self-assigned this Oct 9, 2024
}
)
@ParameterizedTest
public void remoteConfigVars(String paramName) {
Copy link
Member

Choose a reason for hiding this comment

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

🔴 missing test body

Copy link
Member

@jlorper jlorper left a comment

Choose a reason for hiding this comment

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

missing test body

@@ -61,6 +61,10 @@ public SecretManagerConfigService(@Assisted("projectId") @NonNull String project

@Override
public void putConfigProperty(ConfigProperty property, String value) {
if (property.isEnvVarOnly()) {
Copy link
Member

Choose a reason for hiding this comment

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

Tip: I like preconditions for these kind of checks.
Preconditions.checkArgument(!property.isEnvVarOnly(), "Can't put env-only config property: " + property);

Copy link
Contributor

@aperez-worklytics aperez-worklytics left a comment

Choose a reason for hiding this comment

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

OK after @jlorper changes

@eschultink eschultink requested a review from jlorper October 21, 2024 19:46
@eschultink eschultink merged commit 80173ce into rc-v0.5.0 Oct 23, 2024
26 checks passed
@eschultink eschultink deleted the s185-env-only-config branch October 23, 2024 20:01
eschultink added a commit that referenced this pull request Dec 4, 2024
* a bunch of config property stuff

* checks for env-var only config

* fix missing test :facepalm

* CR feedback to use preconditions to check args
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

Successfully merging this pull request may close these issues.

3 participants