jacekschae / learn-reitit-course-files

🎦 Learn Reitit course files for building Cheffy REST API
https://www.learnreitit.com
37 stars 16 forks source link

Port from environment override does not work #12

Open sharms opened 2 years ago

sharms commented 2 years ago

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

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

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

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