kshitij10496 / hercules

The mighty hero helping you build projects on top of IIT Kharagpur's academic data
https://hercules-10496.herokuapp.com/api/v1/static/index.html
MIT License
34 stars 18 forks source link

Add envconfig to project #9

Closed rafaelhenrique closed 5 years ago

rafaelhenrique commented 5 years ago

Trying solve issue #6 :smile:

kshitij10496 commented 5 years ago

This looks good @rafaelhenrique 😄 🎉

Just a few suggestions to make this ready for merge:

  1. As @hypnoglow mentioned above, we can provide a required flag to enforce that both the variables are set. 👏
  2. Use a default value for PORT(say 8080) which can be simply set using the default tag.
  3. I would appreciate it very much if you can add a description using the desc tag for each of the variables.
  4. Define the Config struct in common package and import it in server.go.
  5. Process the input as:
    if err := envconfig.Process("hercules", &config); err != nil {
        fmt.Fprintln(os.Stderr, err)
        envconfig.Usage("hercules", &config)
        os.Exit(1)
    }
rafaelhenrique commented 5 years ago

Applied changes @kshitij10496 @hypnoglow ... thanks for your tips :wink:

kshitij10496 commented 5 years ago

Great work @rafaelhenrique! 🎉

I will wait for @hypnoglow's review before merging it in. 😄

kshitij10496 commented 5 years ago

Thanks, @rafaelhenrique, and @hypnoglow for your invaluable contributions! 🎉 🤗

rafaelhenrique commented 5 years ago

:tada:

kshitij10496 commented 5 years ago

UPDATE:

I deployed the changes to Heroku only to later realise that the platform sets the PORT environment variable for each application. What this means is that it won't update the new HERCULES_PORT variable. Thus, I had to force the envconfig to recognize PORT and read it's value into config.Port variable. 😅 I will try to be more aware of these subtleties in the future.