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

Fix deprecation warnings from tests #3220

Merged
merged 7 commits into from
Aug 15, 2024
Merged

Conversation

mbichoffe
Copy link
Contributor

@mbichoffe mbichoffe commented Jun 13, 2024

I have completed some work on Case Manager that's ready for review!
This pull request makes the following changes:

1) Set action_dispatch.show_exceptions to :none

The deprecation warning:
Setting action_dispatch.show_exceptions to false is deprecated. Set to :none instead. (called from block (4 levels) in class:LinesControllerTest at /home/runner/work/dcaf_case_management/dcaf_case_management/test/controllers/lines_controller_test.rb:34)
is due to a change in how Rails handles the action_dispatch.show_exceptions configuration setting.
Setting it to :none will have the same effect as setting it to false in the previous version.

2) Conditional Secret Key Assignment for Test Environment

The secret key for Devise is now explicitly set to Rails.application.secret_key_base in test environments .
This is a temporary workaround for the tests deprecation warning: Rails.application.secrets is deprecated in favor of Rails.application.credentials and will be removed in Rails 7.2. See related issue.

Next Steps

As part of Rails' evolution, accessing secrets via Rails.application.secrets is being deprecated in favor of Rails.application.credentials. This change aims to enhance security and streamline secret management by utilizing encrypted credentials.
I would consider migrating all sensitive credentials from the unencrypted secrets.yml file to Rails' encrypted credentials system (credentials.yml.enc). I can do this on this PR and share the necessary master key securely via SendSafely with a team member who has the requisite Heroku access. This will allow us to set up the environment variables on Heroku securely and complete the migration process.

It relates to the following issue #s:

For reviewer:

  • Adjust the title to explain what it does for the notification email to the listserv.
  • Tag this PR:
    • feature if it contains a feature, fix, or similar. This is anything that contains a user-facing fix in some way, such as frontend changes, alterations to backend behavior, or bug fixes.
    • dependencies if it contains library upgrades or similar. This is anything that upgrades any dependency, such as a Gemfile update or npm package upgrade.
  • If it contains neither, no need to tag this PR.

@mbichoffe mbichoffe marked this pull request as ready for review June 27, 2024 20:37
@mbichoffe mbichoffe changed the title [WIP] Fix deprecation warnings from tests Fix deprecation warnings from tests Jun 27, 2024
xmunoz
xmunoz previously approved these changes Jul 9, 2024
@lomky
Copy link
Member

lomky commented Jul 31, 2024

Wow this one turned out to be a doozy!

I would consider migrating all sensitive credentials from the unencrypted secrets.yml file to Rails' encrypted credentials system (credentials.yml.enc). I can do this on this PR and share the necessary master key securely via SendSafely with a team member who has the requisite Heroku access. This will allow us to set up the environment variables on Heroku securely and complete the migration process.

We only store dev & test in config/secrets - the production key is stored in the heroku env. Can we just move the dev/test secrets over to credentials and do the same env lookup from it?

@lomky lomky requested a review from colinxfleming July 31, 2024 02:56
@lomky lomky dismissed xmunoz’s stale review July 31, 2024 02:56

Wanna get colin's take on next step before we merge!

Copy link
Member

@colinxfleming colinxfleming left a comment

Choose a reason for hiding this comment

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

I'm good on these changes in this PR! Thank you so much for running this all down.

It's mad annoying that they're deprecating secrets but what can you do. Our secrets file is basically 'two random pieces of garbage plus a ref to an environment variable'; I'd be interested in whether the shift to credentials.yml is as simple as moving it over, and if we can stil have the erb ENV['SECRET_KEY_BASE'] in there. Do you have any sense of that @mbichoffe ?

Either way, I'm good with the diffs in here, so @lomky please punch merge when you're ready!

@lomky lomky merged commit 4c99cf2 into DARIAEngineering:main Aug 15, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rails deprecation warnings
4 participants