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

Allow CLJ_WATSON_* env vars for CI-friendly properties #104

Closed
seancorfield opened this issue Aug 16, 2024 · 4 comments
Closed

Allow CLJ_WATSON_* env vars for CI-friendly properties #104

seancorfield opened this issue Aug 16, 2024 · 4 comments
Assignees
Labels
documentation Improvements or additions to documentation enhancement New feature or request
Milestone

Comments

@seancorfield
Copy link
Contributor

Split off from #66 cc @lread

Search all of the user's env vars, for any that start with CLJ_WATSON_, map the rest of it to a property name (lowercase, _ -> .), and then set that dynamically as a property name.

I believe this will cause env vars to override any JVM properties on the java/clojure CLI invocation?

Document the new behavior.

@lread
Copy link
Contributor

lread commented Aug 17, 2024

I think that's right, except JVM properties on invocation should override env vars, right?

@seancorfield seancorfield added this to the 6.0 milestone Aug 17, 2024
@seancorfield
Copy link
Contributor Author

#66 initially said that, but when the handling of env vars was considered by calling .setProperty() programmatically, I think that would reverse it and the final override cascade reflected that.

If env vars should not override JVM properties set at invocation, then the logic around calling .setProperty() needs to first ensure that .getProperty() returns nil, so it would only conditionally set the property.

I'm open to both approaches -- do you have a strong preference?

I guess (System/setProperty prop (System/getProperty prop env-value)) for CLJ_WATSON_<PROP>=<env-value> would be the concise way to have properties take precedence over env vars? If we do that, #66 should be updated to reflect that.

@lread
Copy link
Contributor

lread commented Aug 17, 2024

Well, I think it might be conventional to be able to override the environment from the command line?

I'll paste my experiment but annotate it:

What's my time zone?

$ clojure -M -e "(-> (java.util.TimeZone/getDefault) .getDisplayName)" 
"Eastern Standard Time"

Let's override that with the TZ environment variable

$ TZ="US/Hawaii" clojure -M -e "(-> (java.util.TimeZone/getDefault) .getDisplayName)"
"Hawaii-Aleutian Standard Time"

And now let's override the environment with the command line:

$ TZ="US/Hawaii" clojure -J-Duser.timezone="UTC" -M -e "(-> (java.util.TimeZone/getDefault) .getDisplayName)"
"Coordinated Universal Time"

@seancorfield
Copy link
Contributor Author

Yeah, I asked Copilot (MS Copilot) so it could summarize from web results with footnotes/links, and then I looked for similar questions on StackOverflow, and the consensus is definitely that JVM properties should take precedence over env vars. I'll update the cascade in #66 accordingly. Thanks.

seancorfield added a commit that referenced this issue Aug 19, 2024
@seancorfield seancorfield self-assigned this Aug 19, 2024
@seancorfield seancorfield added documentation Improvements or additions to documentation enhancement New feature or request labels Aug 19, 2024
lread added a commit to lread/clj-watson that referenced this issue Aug 20, 2024
…-line-arg

* upstream/main:
  address clj-holmes#103 Streamline dependency(-)check.properties overrides (clj-holmes#106)
  fixes clj-holmes#104 by supporting properties via environment variables (clj-holmes#108)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request
Development

No branches or pull requests

2 participants