Add new Rails/EnvironmentVariableAccess cop

[Drenmi]

Background

In my team, we made a decision to stop accessing ENV directly from within application code. We did this because of primarily two reasons:

1. We had no place where we could see all the required ENV variables at one glance.
2. Sometimes we would have runtime errors because of missing configuration.

Instead, we added an environment.yml file, and we load it in our application.rb using Rails' #config_for helper, and copy all the key-value pairs into the Rails configuration object. This solves both our problems, as environment.yml gives an at-a-glance reference for the variables, and any missing configuration causes a failure at boot-time, rather than at runtime.

What is this change

This is the next step. Enforcement. We currently have no means of preventing ENV access from slipping into our application code, which means it will happen. 🙂

This PR adds a cop that flags all access to ENV inside /app and /lib (excluding rake tasks.)

---

Before submitting the PR make sure the following are checked:

• Wrote good commit messages.
• Commit message starts with [Fix #issue-number] (if the related issue exists).
• Feature branch is up-to-date with master (if not - rebase it).
• Squashed related commits together.
• Added tests.
• Added an entry to the Changelog if the new code introduces user-observable changes. See changelog entry format.
• The PR relates to only one subject with a clear title
  and description in grammatically correct, complete sentences.
• Run bundle exec rake default. It executes all tests and RuboCop for itself, and generates the documentation.
• If this is a new cop, consider making a corresponding update to the Rails Style Guide.

[koic]

I saw case of assigning to environment variable required by library (e.g.: ENV['REQUIRED_BY_LIBRARY'] = app_specific_value). In other words, this cop will prohibit only reference to ENV. How about allowing assignments to ENV?

[koic]

I came up with a mundane idea of splitting the cop by reference and assignment to ENV, or providing options :-)

[Drenmi]

I saw case of assigning to environment variable required by library (e.g.: ENV['REQUIRED_BY_LIBRARY'] = app_specific_value). In other words, this cop will prohibit only reference to ENV. How about allowing assignments to ENV?

I can add a configuration option. In the interest of time, I think I will limit to #= and assume ENV is always flat (not nested.) 🙂

[koic]

I think I will limit to #= and assume ENV is always flat (not nested.)

I think it's good enough :-)

[Drenmi]

@koic Added!

[Drenmi]

Not sure why JRuby is failing. Seems unrelated to the change.

[koic]

@Drenmi Can you rebase with the latest master branch and update this PR?

[Drenmi]

Will have a look this week. 🙏

[koic]

@Drenmi I plan to release new RuboCop Rails in a few days. Is it possible to make progress this PR?

[Drenmi]

Sorry for the delay, @koic. I've been caught up with work. I just rebased this and incorporated the suggested documentation.

[koic]

@Drenmi Thank you for updating! 😃

[swrobel]

@Drenmi how do you feel about ENV.fetch() with a fallback? ex: ENV.fetch('UNDEFINED_ENV_VAR', 'will return this'). I just upgraded and this cop is currently flagging these calls, but I don't see them as problematic because they don't cause runtime errors.

[olliebennett]

Hey, @swrobel. I initially had this response, but I've come around to the idea and am changing my code to comply with the cop. You're right that fetch with fallback wouldn't cause runtime errors, however the secondary benefit that would be missed (if fetch with fallback was permitted) is (from above);

We had no place where we could see all the required ENV variables at one glance.

I find this really handy; it informs which variables my environment needs, without needing to rummage through the whole codebase to check if there are any ENV.fetch, etc. (that I wouldn't otherwise notice until runtime) and prevents me accidentally forgetting to add a new (optional or required) env var to a .env.template file which is commonly used with dotenv.

You could of course use this to have the same effect if helpful;

# in initializer or config setup
Rails.application.config.my_undefined_env_var = nil
# deep in code somewhere
Rails.application.config.my_undefined_env_var || 'will return this'

What do you think?

[jeppester]

While I'm not against this cop itself, it was an unpleasant surprise to stumble upon after a rubocop-rails update.

I need to invent my own thing to make it pass, and while that's surely possible - based on the PR-description and comments - I don't think it's ideal.

There need to be a corresponding best practice example for this cop, in the Rails Style Guide, or somewhere else. As it is now I fear this cop will mostly lead to fragmentation and wasted time.

[citrus]

@Drenmi can you provide example code for how you're loading your env vars via environment.yml? While this cop does provide reasoning behind its purpose, it doesn't give developers a migration path.

[koic]

@Drenmi Could you write a migration guide to a good case? Probably due to user confusion, I'm considering disabling the cop by default if migration explanation doc is not yet in place by the next release.