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

Dotenv support #469

Merged
merged 10 commits into from May 22, 2018

Conversation

@rbndelrio
Copy link
Contributor

commented Apr 17, 2018

Hey, so the company I'm working for has been using Wordmove fairly heavily for the last year but version controlling Movefiles and credential/secret management have had some growing pains:

Should Movefiles be comitted? If so, how should devs reconcile development workflow differences (VVV/Docker/etc.)? And what does this mean for secrets like randomized db names and passwords? Up a few weeks ago, our team members have been doing a mix of committing secrets, overwriting each others' local configs, and copy-pasting Movefiles over Slack, all of which are pretty bad for security and maintainability.

Committing a "censored" movefile.yml.example is one way forward, but gets difficult to manage across a team if there are changes to vhosts/hooks/mysqldump options/etc..

So in this fork, I've added support for environment variables for internal use. It allows us to commit movefile.yml files to our repos without passing sensitive data. The feature allows users to either load .env files or environment-specific files. For example, wordmove push -e staging will look for .env.staging. This also plays well with the PHP and Node versions of dotenv, so you only have to specify things like WP_HOME and local development port once across the entire project.

I've added a more detailed use case to the README.

I've honestly never worked with Ruby before, so I'd love any assistance with getting this PR to a more polished state!

rbndelrio added 3 commits Mar 23, 2018
Copy link
Member

left a comment

Hi @rbndelrio .

I'm really really really glad to read your contribution. I'm glad this could be useful for your company and your teammates. I'm glad this is a really smart feature. I'm glad you never wrote in ruby, but you managed to understand and implement the code.

I have a couple of questions and few advices about the code, which I'd like to discuss directly in the code and I hope you'll find them relevant.

Copy link
Member

left a comment

That said, we'll need to add some tests here, but we can do that later and I could help you doing that.

In `bash`, you can set variables like so:
```bash
export PROD_DB_USER="username" PROD_DB_PASS="password"
```

This comment has been minimized.

Copy link
@pioneerskies

pioneerskies Apr 18, 2018

Member

while in fish you can use

set --export --global PROD_DB_USER "username"; set --export --global PROD_DB_PASS "password"

This comment has been minimized.

Copy link
@pioneerskies

pioneerskies Apr 18, 2018

Member

I know that fish is not the standard, but since I'm a fish user, I like to support it in documentation :)


### Dotenv
Wordmove supports the [dotenv](https://github.com/bkeepers/dotenv) module. Simply create a file named `.env` next to your movefile:
```dosini

This comment has been minimized.

Copy link
@pioneerskies

pioneerskies Apr 18, 2018

Member

Just add a newline before code block (and do not hate me for this useless and compulsive comment! 😝 )

PROD_DB_USER=username
PROD_DB_PASS=password
```
You may also use `.env.{environmentname}`, but this is discouraged.

This comment has been minimized.

Copy link
@pioneerskies

pioneerskies Apr 18, 2018

Member

Just to be more verbose I'd write

You may also use .env.{environmentname}, where environmentname is one of the movefile.yml environment you configured, but this is discouraged.

@@ -11,6 +11,7 @@
require 'active_support'
require 'active_support/core_ext'
require 'kwalify'
require 'dotenv'

This comment has been minimized.

Copy link
@pioneerskies

pioneerskies Apr 18, 2018

Member

Not your fault, but requires block should be internally ordered:

require 'active_support'
require 'active_support/core_ext'
require 'colorize'
require 'erb'
require 'kwalify'
require 'logger'
require 'open-uri'
require 'ostruct'
require 'thor'
require 'thor/group'
require 'yaml'

If you have 2 seconds more, would be nice to do this

@@ -26,6 +26,17 @@ def fetch(verbose = true)
YAML.safe_load(ERB.new(File.read(found)).result, [], [], true).deep_symbolize_keys!
end

def dotenv(cli_options = {})

This comment has been minimized.

Copy link
@pioneerskies

pioneerskies Apr 18, 2018

Member

Since this is a method which does not return a value, we couldn't name it as a getter-like method: if I read environment I expect an environment will be returned; if I read dotenv I expect a dotenv will be returned.

What this method does is just to fire an actin, thus I'd name it with verb such as e.g. load_dotenv.

@@ -26,6 +26,17 @@ def fetch(verbose = true)
YAML.safe_load(ERB.new(File.read(found)).result, [], [], true).deep_symbolize_keys!
end

def dotenv(cli_options = {})
env = cli_options['environment'] || cli_options[:environment]

This comment has been minimized.

Copy link
@pioneerskies

pioneerskies Apr 18, 2018

Member

Two notes:

  1. as it stands now, cli_options should not be an optional argument because here env could be nil (or raise some error?). Do you agree? But let's read the next...
  2. you wrote custom logic in order to read env from cli_options, but in the very same Wordmove::Movefile class you have a getter-like method environment that already does that and it does with more complex logic. So IMHO this line should become
env = environment(cli_options)

Note that you can directly call the method since you are in the same class.

This comment has been minimized.

Copy link
@rbndelrio

rbndelrio Apr 19, 2018

Author Contributor

That makes sense. I believe my goal with that was to avoid unnecessary function calls. It does seem to be a bit of a footgun for stability, though. I'll go ahead and use your version.


found_env = env_files.first
logger.task("Using .env file: #{found_env}") if found_env && found_env != ENV['dotenv']
ENV['dotenv'] = found_env

This comment has been minimized.

Copy link
@pioneerskies

pioneerskies Apr 18, 2018

Member

Would you mind to explain to me how you think ENV['dotenv'] could be sued/useful?

At first glance I'd have wrote

found_env = env_files.first

return false unless found_env.present?

logger.task("Using .env file: #{found_env}")
Dotenv.load(found_env) # I hope .load will return true/false thus having a consistent return value

But I'm afraid I'm missing something from your reasoning.

This comment has been minimized.

Copy link
@rbndelrio

rbndelrio Apr 19, 2018

Author Contributor

My reasoning was to serve a dual purpose:

  • A quasi-immutable value to ensure stdout doesn't print the "Using .env file" statement more than once (something I ran into a few times when developing the feature).
  • Saving the path for the dotenv could be useful to some users trying to extend the functionality of the movefile like so:
hooks:
  push:
    before:
      local:
        - 'yarn run build --env="<%= ENV['dotenv'] %>"'

...but after writing it out, I realize the first point is counter-productive, and the second is pointless without actually documenting it. Plus, it's kinda enabling some potentially bad habits. Removed!

@pioneerskies

This comment has been minimized.

Copy link
Member

commented May 5, 2018

You got a PR on your fork that should complete this really nice work

add spec for .load_dotenv method
@rbndelrio

This comment has been minimized.

Copy link
Contributor Author

commented May 21, 2018

Two weeks of dead air later, I merged your pull request @pioneerskies

By the way, thanks for helping me get a grasp on Ruby development! I'll make sure to pay it forward next time someone could use the patience.

@pioneerskies pioneerskies merged commit d098cf8 into welaika:master May 22, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@pioneerskies

This comment has been minimized.

Copy link
Member

commented May 22, 2018

Nice. We are in master. I will do a check to the docs and a release as soon as possible :)

Thank you very very much again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.