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

Do not overwrite shell declared ENV vars #715

Merged
merged 1 commit into from Jan 27, 2017

Conversation

Projects
None yet
2 participants
@gizotti
Copy link
Contributor

gizotti commented Jan 26, 2017

As mentioned on #712, if an ENV var, with the same name as one declared on .env.* is declared on the shell when calling a hanami command, we will end up with the value declared on `.env.*, which is not the desired outcome.

This PR changes the initialization of Hanami::Environment to ensure that the ENV vars declared by the user on the shell are not overwritten when .env.* is loaded.

While my solutions works, there are some I would like to discuss if it is the best solution or not.

My other solution was to change Hanami::Env#load! to only load the variables that yet not loaded on the instance. Now that I am writing this PR and thinking better, this is actually a better solution then the actual, but I wait for some feedback before I act on it.

Closes #712

@jodosha

This comment has been minimized.

Copy link
Member

jodosha commented Jan 26, 2017

@gizotti Hey Gabriel thanks for this PR.

Probably delegating this responsibility to Hanami::Env is better because this accidental override happens only when .env.* files are loaded.

Can you please verify it it's possible? Thanks! 👍

@gizotti

This comment has been minimized.

Copy link
Contributor Author

gizotti commented Jan 26, 2017

@jodosha no worries at all.

I was just having a look at it and it's as simple as skipping the assigning of the variable on Hanami::Env#load! if that variable is already loaded in the instance.
I can add an allow_overwrite flag to the #load! method, but I am not sure if it's that important right now.

@gizotti gizotti force-pushed the gizotti:shell-env-variables branch from 5f7715f to 8d4ab2d Jan 26, 2017

@jodosha jodosha self-assigned this Jan 27, 2017

@jodosha jodosha added the fix label Jan 27, 2017

@jodosha jodosha added this to the v1.0.0.beta1 milestone Jan 27, 2017

@jodosha
Copy link
Member

jodosha left a comment

👍 LGTM - Thanks!

@jodosha jodosha merged commit 42b6307 into hanami:master Jan 27, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jodosha

This comment has been minimized.

Copy link
Member

jodosha commented Jan 27, 2017

@gizotti

I can add an allow_overwrite flag to the #load! method, but I am not sure if it's that important right now.

I avoid boolean flags in methods because if you look closer they are a symptom of a SRI violation. A method supposed to do X, unless the flag is true, then it does Y. That means too many responsibilities for a method. 😉

The PR is good as it is, merged. Thank you very much! 👍

@gizotti

This comment has been minimized.

Copy link
Contributor Author

gizotti commented Jan 27, 2017

@jodosha

I avoid boolean flags in methods because if you look closer they are a symptom of a SRI violation. A method supposed to do X, unless the flag is true, then it does Y. That means too many responsibilities for a method. 😉

Never really thought about it this way. Thanks for the feedback!

@gizotti gizotti deleted the gizotti:shell-env-variables branch Jan 27, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment