[1422] Create helper method to check ENV variables by esoterik · Pull Request #1583 · YaleSTC/reservations

esoterik · 2016-06-13T17:53:41Z

Resolves #1422

orenyk · 2016-06-14T03:43:56Z

config/database.yml.example.production

 staging:
  adapter: mysql2
  encoding: utf8
-  database: <%= ENV['RES_DB_NAME'] %>


These are pulling in the values themselves, not just checking them as booleans, so they shouldn't be replaced.

orenyk · 2016-06-14T03:59:43Z

I like how this was implemented; that said, I started going through this and found a bunch of places where the custom method was used incorrectly (e.g. assigning things to variables vs checking a boolean). I also think we should figure out a better place to put the helper method, so let's chat about the latter point and why don't you do a quick pass through this to check for any other cases where you replaced an ENV call with the helper incorrectly. Thanks!

esoterik · 2016-06-14T14:26:04Z

Ah, the dangers of using sed to replace everything and not checking it thoroughly enough--I'll go through it

esoterik · 2016-06-14T16:23:27Z

config/initializers/party_foul.rb.example


  # The OAuth token for the account that is opening the issues on GitHub
-  config.oauth_token            = ENV['PARTY_FOUL_TOKEN']
+  config.oauth_token            = env?('PARTY_FOUL_TOKEN')


esoterik · 2016-06-14T16:51:20Z

Okay, ready for rereview!

esoterik · 2016-06-14T16:52:19Z

config/initializers/00_devise.rb

@@ -1,3 +1,4 @@
+include EnvironmentHandler


this is how it gets included everywhere--I'm not sure if this is the best solution

Let's put it in its own initializer - they run in alphabetical order so I'd call it something like config/initializers/000_env_helper.rb or whatever will place it before the Devise initializer.

orenyk · 2016-06-21T02:01:13Z

A few more inline comments, I'm particularly concerned by config/environments/production.rb, we have a few boolean flags but I don't know if there's a way to load the helper before it gets processed. Let me know what you think!

esoterik · 2016-09-25T14:42:39Z

Pushed an update resolving the issues with the production config. I think this will be good to merge after the build passes

orenyk · 2016-10-28T19:39:01Z

So basically we should remove the change from #1618 when this is merged in and instead add a committed .env.test file.

orenyk

A few things need to be fixed here, and I think we should probably include an empty string as "falsey". Let me know what you think!

orenyk · 2016-11-14T20:03:58Z

config/environments/production.rb

  # Disable serving static files from the `/public` folder by default since
  # Apache or NGINX already handles this.
-  config.serve_static_files = ENV['RAILS_SERVE_STATIC_FILES'].present?
+  config.serve_static_files = !FALSE.include?(ENV['RAILS_SERVE_STATIC_FILES'])


Shouldn't this be ENV_FALSE.include?? Same below

orenyk · 2016-11-14T20:04:26Z

config/environments/production.rb

@@ -1,5 +1,7 @@
 # frozen_string_literal: true
 Rails.application.configure do
+  ENV_FALSE = [0, '0', false, 'false', nil].freeze


Should we also include an empty string?

orenyk · 2016-11-14T20:05:53Z

config/environments/production.rb


  # Disable e-mails if environment variable is set
-  config.action_mailer.perform_deliveries = ENV['DISABLE_EMAILS'].nil?
+  config.action_mailer.perform_deliveries = \


I believe that this should be inverted.

orenyk · 2016-11-14T20:07:09Z

lib/environment_handler.rb

@@ -0,0 +1,8 @@
+# frozen_string_literal: true
+module EnvironmentHandler
+  FALSE = [0, '0', false, 'false', nil].freeze


Same here, do we want to include an empty string?

orenyk · 2016-11-14T20:08:51Z

spec/lib/environment_handler_spec.rb

+include EnvironmentHandler
+
+describe EnvironmentHandler do
+  describe '.env?' do


As stated above, I think that we should include an empty string as being "falsey". We should also add a test for that.

orenyk · 2016-11-15T01:31:21Z

FYI, this also needs to be rebased.

Resolves #1422

esoterik · 2016-11-20T21:05:18Z

updated!

orenyk

One of the specs for the seed generators is failing and I think there's also one inverted env? call. Once those two are resolved we should be good to go. It would probably also be good if you could do one more complete read-through to make sure I didn't miss any other inversions that aren't handled by testing.

orenyk · 2016-11-22T12:29:46Z

config/initializers/authentication.rb

  # if we want to use password authentication all users can reset their
  # passwords so it doesn't matter if they already have them or not
-  elsif ENV['CAS_AUTH'].nil? && user && (user.username != user.email)
+  elsif env?('CAS_AUTH')


Shouldn't this be inverted?

esoterik added the 0 - needs review label Jun 13, 2016

orenyk reviewed Jun 14, 2016
View reviewed changes

orenyk added 1 - working and removed 0 - needs review labels Jun 14, 2016

esoterik changed the title ~~Create helper method to check ENV variables~~ [1422] Create helper method to check ENV variables Jun 14, 2016

esoterik reviewed Jun 14, 2016
View reviewed changes

esoterik added 0 - needs review and removed 1 - working labels Jun 14, 2016

esoterik reviewed Jun 14, 2016
View reviewed changes

orenyk added 1 - working and removed 0 - needs review labels Jun 21, 2016

esoterik force-pushed the 1422_fix_env_behavior branch from c206d50 to efb539f Compare June 21, 2016 21:09

esoterik added the pri: 1 label Jun 29, 2016

esoterik added the diff: 1 label Aug 25, 2016

esoterik added pri: 2 and removed pri: 1 labels Sep 12, 2016

orenyk self-assigned this Sep 19, 2016

esoterik force-pushed the 1422_fix_env_behavior branch from 1521dd9 to 5e132f4 Compare September 25, 2016 14:36

esoterik added 0 - needs review and removed 1 - working labels Sep 25, 2016

orenyk added 0 - blocked and removed 0 - blocked labels Oct 28, 2016

orenyk suggested changes Nov 14, 2016

View reviewed changes

orenyk added 1 - working and removed 0 - needs review labels Nov 15, 2016

Create helper method to check ENV variables

771dbbe

Resolves #1422

esoterik force-pushed the 1422_fix_env_behavior branch from 5e132f4 to 771dbbe Compare November 20, 2016 21:03

esoterik added 0 - needs review and removed 1 - working labels Nov 20, 2016

orenyk suggested changes Nov 22, 2016

View reviewed changes

orenyk added 1 - working and removed 0 - needs review labels Nov 22, 2016

Conversation

esoterik commented Jun 13, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

orenyk commented Jun 14, 2016

Uh oh!

esoterik commented Jun 14, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

esoterik commented Jun 14, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

orenyk commented Jun 21, 2016

Uh oh!

esoterik commented Sep 25, 2016

Uh oh!

orenyk commented Oct 28, 2016

Uh oh!

orenyk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

orenyk commented Nov 15, 2016

Uh oh!

esoterik commented Nov 20, 2016

Uh oh!

orenyk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants