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

MF-435 - Add support for env file loading #1223

Merged
merged 3 commits into from
Sep 7, 2020
Merged

Conversation

hoanga
Copy link
Contributor

@hoanga hoanga commented Jul 20, 2020

Signed-off-by: Al Hoang [email protected]

What does this do?

Adds support for loading environment variables from a file path

Which issue(s) does this PR fix/relate to?

This pull request resolves #435.

List any changes that modify/break current functionality

Have you included tests for your changes?

n/a

Did you document any new/modified functionality?

yes

Notes

@hoanga hoanga requested a review from a team as a code owner July 20, 2020 01:56
@dborovcanin dborovcanin changed the title NO-ISSUE - add support for env file loading NOISSUE - Add support for env file loading Jul 20, 2020
Copy link
Collaborator

@dborovcanin dborovcanin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I like the idea, have you considered using a different lib (such as https://github.com/joho/godotenv) or writing your simple parser? I don't like adding a new dependency, especially since this one does not look well maintained.

@manuio
Copy link
Contributor

manuio commented Jul 20, 2020

@hoanga Let's use this issue as reference for this PR: https://github.com/mainflux/mainflux/issues/435

@hoanga
Copy link
Contributor Author

hoanga commented Jul 20, 2020

@dusanb94 one of the underlying config file parsing libraries references this library in this pr which is the basis for choosing the library in question. I understand the concern regarding keeping the number of introduced dependencies low, but with the availability of libraries that fall right into this domain, if the footprint is small enough, I would suggest considering using a more battle-tested library for this use case instead of re-implementing a (possible) buggy rewrite.

Both this env parsing library and godotenv are part of the dependency graph for this project from what I can discern from inspection. I've setup an alternate version using godotenv (see #1224). The API should remain the same, the underlying implementation is not.

@dborovcanin
Copy link
Collaborator

dborovcanin commented Jul 21, 2020

@dusanb94 one of the underlying config file parsing libraries references this library in this pr which is the basis for choosing the library in question. I understand the concern regarding keeping the number of introduced dependencies low, but with the availability of libraries that fall right into this domain, if the footprint is small enough, I would suggest considering using a more battle-tested library for this use case instead of re-implementing a (possible) buggy rewrite.

Both this env parsing library and godotenv are part of the dependency graph for this project from what I can discern from inspection. I've setup an alternate version using godotenv (see #1224). The API should remain the same, the underlying implementation is not.

@hoanga OK, makes sense. I'll close the other PR, let's keep only this. It's easier to follow the conversation. Can you add an example of using this function? Also, please check dependencies, CI is failing.

@dborovcanin dborovcanin changed the title NOISSUE - Add support for env file loading MF-435 - Add support for env file loading Jul 21, 2020
@drasko
Copy link
Contributor

drasko commented Jul 26, 2020

@hoanga can you please proceed on thi PR as @dusanb94 explained? Otherwise I will close it due to inactivity.

Signed-off-by: Al Hoang <[email protected]>
@codecov-commenter
Copy link

codecov-commenter commented Jul 28, 2020

Codecov Report

Merging #1223 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1223   +/-   ##
=======================================
  Coverage   76.18%   76.18%           
=======================================
  Files         106      106           
  Lines        6983     6983           
=======================================
  Hits         5320     5320           
  Misses       1278     1278           
  Partials      385      385           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 26c944b...685ba46. Read the comment docs.

@hoanga
Copy link
Contributor Author

hoanga commented Jul 28, 2020

@dusanb94 @drasko I have put in updates to update the branch to track latest. Below is a sample calling this function as the first steps in initialization of a program that uses environment settings to:


package main

import (
        "github.com/mainflux/mainflux"
)

const defaultEnvPath = ".env"

func main() {
        if err := loadEnvironment(); err != nil {
                os.Exit(1)
        }
        // .. Further initializations here

        // .. Add your business logic here
}

func loadEnvironment() error {
        // Populate environment vars from file
        err := mainflux.LoadEnvFile(defaultEnvPath)
        if err != nil {
                return error
        }
}

@dborovcanin
Copy link
Collaborator

dborovcanin commented Jul 28, 2020

@dusanb94 @drasko I have put in updates to update the branch to track latest. Below is a sample calling this function as the first steps in initialization of a program that uses environment settings to:


package main

import (
        "github.com/mainflux/mainflux"
)

const defaultEnvPath = ".env"

func main() {
        if err := loadEnvironment(); err != nil {
                os.Exit(1)
        }
        // .. Further initializations here

        // .. Add your business logic here
}

func loadEnvironment() error {
        // Populate environment vars from file
        err := mainflux.LoadEnvFile(defaultEnvPath)
        if err != nil {
                return error
        }
}

@hoanga When I asked for a code example, I meant that you actually use this function in the main methods of Mainflux services. You can paste this code sample to main.go files in cmd. Also, you can move const defaultEnvFilePath = ".env" to env.go and modify LoadEnv to:

// LoadEnvFile loads environment variables defined in an .env formatted file.
func LoadEnvFile(envFilePath string) error {
	if envFilePath == "" {
            envFilePath = defEnvFilePath
        }
        return gotenv.Load(envfilepath)
}

@drasko
Copy link
Contributor

drasko commented Jul 31, 2020

@hoanga can you please take a look at these remarks from @dusanb94

@hoanga
Copy link
Contributor Author

hoanga commented Aug 1, 2020

@dusanb94 I'm still unclear on what you mean when you are asking for a code sample. If you are asking me to modify all of the existing mainflux services main() method to include this change, that sounds like it would be better handled in the scope of another PR (perhaps with tests). The intent for this PR is to put in the facilities to allow loading from an env file and enable services that could use env file loading as needed.

It would make more sense to show how this could be done for a brand new service that could illustrate how to use this but what new service that would be I feel is also outside of the scope of this current PR. If the mainflux authors happen to have a suggestion/direction for such a brand new service it might make sense to comment in that future PR on how to use this.

Originally, I thought having this method call would be beneficial for contexts I was working with and the mainflux maintainers as well. I will leave it up to the project maintainers how to handle this PR at this point.

@manuio
Copy link
Contributor

manuio commented Aug 4, 2020

@hoanga @dusanb94 We can add here: https://github.com/mainflux/mainflux/blob/master/env.go#L12 something similar toos.Getenv(key) but to read the key from a .env file. It would avoid to modify any main.go.

@drasko
Copy link
Contributor

drasko commented Aug 26, 2020

@dusanb94 please take a look at this urgently.

Copy link
Contributor

@manuio manuio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@manuio manuio merged commit e272d9f into absmach:master Sep 7, 2020
manuio pushed a commit that referenced this pull request Oct 12, 2020
* NO-ISSUE - add support for env file loading

Signed-off-by: Al Hoang <[email protected]>

* update go mod

Signed-off-by: Al Hoang <[email protected]>

Co-authored-by: Dušan Borovčanin <[email protected]>
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

Successfully merging this pull request may close these issues.

Enable service configuration by using configuration files
5 participants