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

Add rails credentials support #355

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

noxasch
Copy link

@noxasch noxasch commented Mar 8, 2024

Added rails credentials support with config flag addressing #68

@noxasch noxasch force-pushed the feat/add-rails-credentials-support branch from a24c238 to 61810ff Compare March 8, 2024 20:09
@pkuczynski pkuczynski added this to the Next milestone Mar 9, 2024
@noxasch
Copy link
Author

noxasch commented Mar 14, 2024

I'm currently having trouble with different ruby version in the test, any clue ?

@cjlarose
Copy link
Member

Not 100% sure what's going on with the tests on CI. Tests pass for me locally. I suspect it has something to do with Rails 7.0 or 7.1 because we don't run tests for those Rails versions when running the test suite for Ruby 2.7, jruby, or truffleruby.

I merged in a change that address the deprecation warnings for fixture_path in Rails 7.1 to help clear up the test output. Hoping that will help clear things up 🤞

lib/config.rb Outdated Show resolved Hide resolved
@noxasch noxasch force-pushed the feat/add-rails-credentials-support branch 9 times, most recently from 075d284 to ef7ccc8 Compare March 16, 2024 12:17
@noxasch
Copy link
Author

noxasch commented Mar 16, 2024

Update:

Rebased to latest master

I look around and found the solution for rails 7.1 fail test. For Rails 7.1 and above it seems we need to use ActiveSupport::EncryptedConfiguration config method instead. But prior 7.1, Rails.credentials.to_h and Rails.secret.to_h will work just fine.

All test should pass now.

@noxasch noxasch requested a review from cjlarose March 16, 2024 12:18
Copy link
Member

@cjlarose cjlarose left a comment

Choose a reason for hiding this comment

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

Awesome work! Requested a few changes

lib/config.rb Outdated Show resolved Hide resolved
lib/config.rb Outdated Show resolved Hide resolved
lib/config.rb Outdated Show resolved Hide resolved
lib/config.rb Outdated Show resolved Hide resolved
spec/app/rails_7.1/config/credentials/test.yml.enc Outdated Show resolved Hide resolved
@noxasch noxasch force-pushed the feat/add-rails-credentials-support branch 8 times, most recently from 36b06f6 to f67fadf Compare March 17, 2024 14:52
@noxasch
Copy link
Author

noxasch commented Mar 17, 2024

Update:

  1. Addressed the changes and refactor the code.
  2. Update the crendentails to include the aws patter below to be more realistic
aws:
  secret_access_key: '123456'

Apparently we don't need to check for rails version, just need to require the master_key in test environment and for rails5.2 untiil 6.1 need test.key and master.key, otherwise it somehow ignore the RAILS_MASTER_KEY in environment variable

@noxasch noxasch requested a review from cjlarose March 18, 2024 00:08
@BuonOmo BuonOmo removed their request for review March 24, 2024 14:28
@noxasch
Copy link
Author

noxasch commented Apr 23, 2024

@cjlarose let me know if there is anything else require.

@noxasch noxasch force-pushed the feat/add-rails-credentials-support branch from 4eddca8 to de1bb93 Compare May 16, 2024 15:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

None yet

3 participants