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

Port from environment override does not work #12

Open
sharms opened this issue Nov 28, 2021 · 3 comments
Open

Port from environment override does not work #12

sharms opened this issue Nov 28, 2021 · 3 comments

Comments

@sharms
Copy link

sharms commented Nov 28, 2021

In increment 11 ig/prep-key :server/jetty is added, however it cannot execute without PORT being explicitly set: https://github.com/jacekschae/learn-reitit-course-files/blob/master/increments/11-integrant-reloaded/src/cheffy/server.clj#L14

If PORT is unset the server will fail to launch with: Execution error (NumberFormatException) at java.lang.Integer/parseInt (Integer.java:630). Cannot parse null string. Additionally merge will always overwrite the key :port set in resources/config.edn.

This may be solved by catching the NumberFormatException and potentially using or should the value be nil

@m1nhtu99-hoan9
Copy link

m1nhtu99-hoan9 commented Jul 21, 2022

This is how I cope with it:

(defmethod ig/prep-key :server/jetty
  [_ config]
  ; in PROD, get the port number from env variables instead of from the config.edn
  (let [port (env :port)]
    (if port
      (merge config {:port (Integer/parseInt port)})
      config)))

The smarter way would be as you suggested:

(defmethod ig/prep-key :server/jetty
  [_ config]
  ; in PROD, get the port number from env variables instead of from the config.edn
  (let [port (env :port)]
    (merge config 
           (and port {:port (Integer/parseInt port)}))))

Personally I prefer the former.

@m1nhtu99-hoan9
Copy link

I still don't think that either of the code snippets I proposed last comment are idiomatic Clojure yet.

After looking around for a while, I think this is the most idiomatic one (with some->> macro coming to rescue 🧐):

(defmethod ig/prep-key :adapter/jetty
  [_ config]
  ; in PROD, get the port number from env variables instead of from the config.edn
  (merge config (some->> env 
                         (:port)
                         (Integer/parseInt)
                         (assoc {} :port))))

@jacekschae
Copy link
Owner

jacekschae commented Aug 7, 2022

I stumbled on this one right now. Of course we could safeguard this in different ways ... I'm trying to understand why? I think it's appropriate at the system start to get this message. What am I missing?

If you really want you could set a default value:

(defmethod ig/prep-key :server/jetty
  [_ {:keys [port] :or {port 3000}]
  (merge config {:port port}))

But I feel like this will beat the purpose of this failing. I think you want to get notified with Execution error that it's not there or something is messed up?

Please if you could elaborate more on this I would like to understand

BTW. The whole config for prod is reworked toward end of the course

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

3 participants