search

clj-holmes / clj-watson

clojure deps SCA
Eclipse Public License 2.0
83 stars 9 forks source link

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

Closed seancorfield closed 2 months ago

seancorfield commented 2 months ago

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 commented 2 months ago

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

seancorfield commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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.