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

Remove ENVied Gem, Use dotenv Take 2 #9767

Merged
merged 3 commits into from Aug 14, 2020
Merged

Conversation

mstruve
Copy link
Contributor

@mstruve mstruve commented Aug 13, 2020

What type of PR is this? (check all applicable)

  • Refactor

Description

This is a followup to #9621. The difference between the original and this is the migration script. Now the migration

  • does NOT remove the application.yml file. This allows users that may have issues with the migration not to lose their keys which @rhymes and I(yeah I did it too when I creating this) were both victims to.
  • It sets default values that may not be present in the application.yml file. These default however have been added to the .env_sample file so going forward they should always be present for users starting fresh.

TODO: Remove sample application yml file and Figaro gem

Related Tickets & Documents

QA Instructions

You dont have to do all of these but if you want to give it a thorough test.

  • Pull down branch
  • run bin/setup
  • Ensure app still runs
  • run bin/setup again to make sure no errors
  • Ensure app still runs
  • force kill app and boot it back up, should still run

alt_text

@pr-triage pr-triage bot added the PR: draft bot applied label for PR's that are a work in progress label Aug 13, 2020

File.open("config/application.yml", 'r') do |file|
file.each_line do |line|
new_line = line.gsub(": ", "=")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rhymes I found with your script the colons within the default URLs got swapped out for = which we dont want so I switched back to this way. I also couldn't find the commit even after fetching all so I went ahead and copied your code into my own commit.

Copy link
Contributor

Choose a reason for hiding this comment

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

Perfect! :-)

@mstruve mstruve marked this pull request as ready for review August 13, 2020 13:38
@mstruve mstruve requested review from a team August 13, 2020 13:38
@mstruve mstruve requested a review from a team as a code owner August 13, 2020 13:38
@mstruve mstruve requested review from michael-tharrington and removed request for a team August 13, 2020 13:38
@pr-triage pr-triage bot added PR: unreviewed bot applied label for PR's with no review and removed PR: draft bot applied label for PR's that are a work in progress labels Aug 13, 2020
Copy link
Contributor

@rhymes rhymes left a comment

Choose a reason for hiding this comment

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

The script apparently works, then I booted the app, clicked around but http://localhost:3000/dashboard crashed and in the logs I can see the following:

ActionView::Template::Error - Must supply api_secret:
  app/views/shell/_top.html.erb:37:in `view template'
  app/controllers/shell_controller.rb:9:in `top'

It looks like it's looking for some Cloudinary configuration...

<link rel="apple-touch-icon" href="<%= cloudinary(SiteConfig.logo_png, 180, "png") %>">

Copy link
Contributor

@rhymes rhymes left a comment

Choose a reason for hiding this comment

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

Tried the script, it worked, the app boots up correctly as well.

I haven't tested the container setup though

@pr-triage pr-triage bot added PR: partially-approved bot applied label for PR's where a single reviewer approves changes and removed PR: unreviewed bot applied label for PR's with no review labels Aug 13, 2020
@mstruve mstruve requested review from citizen428 and removed request for michael-tharrington August 13, 2020 14:53
Copy link
Member

@maestromac maestromac left a comment

Choose a reason for hiding this comment

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

I had to remove some personal commented out env var but this works. On a slightly unrelated note, I got a lot of WARNING .. Key already set which is gone after I remove my application.yml.

Tested and working locally!

@mstruve
Copy link
Contributor Author

mstruve commented Aug 13, 2020

@citizen428 can you poke around the container setup and let me know if you see anything not working? Please and THANK YOU!

@mstruve
Copy link
Contributor Author

mstruve commented Aug 14, 2020

Yeah @maestromac you have to live with those warnings until application.yml is removed manually or when we remove the Figaro gem

@mstruve mstruve merged commit 0a64ac0 into master Aug 14, 2020
@mstruve mstruve deleted the mstruve/rm-envied-take-2 branch August 14, 2020 17:20
@pr-triage pr-triage bot removed the PR: partially-approved bot applied label for PR's where a single reviewer approves changes label Aug 14, 2020
@pr-triage pr-triage bot added the PR: merged bot applied label for PR's that are merged label Aug 14, 2020
@yihyang yihyang mentioned this pull request Oct 29, 2020
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: merged bot applied label for PR's that are merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants