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

deep symbolize keys on secrets.yml #26929

Merged
merged 1 commit into from Nov 21, 2016

Conversation

Projects
None yet
8 participants
@elorest
Contributor

elorest commented Oct 29, 2016

Summary

Changed symbolize_keys to deep_symbolize_keys where secrets.yml is loaded in. This allows secret files to contain more complex information without all child keys being strings while parent keys are symbols.

{:smtp_settings=>{"address"=>"smtp.mailgun.org", "port"=>587, "domain"=>"example.com", "authentication"=>"plain", "enable_starttls_auto"=>true, "user_name"=>"postmaster@mailgun.example.com", "password"=>"12d3f673e0a83c97045eb4e2d10ebc8a4"}}

becomes this

{:smtp_settings=>{:address=>"smtp.mailgun.org", :port=>587, :domain=>"example.com", :authentication=>"plain", :enable_starttls_auto=>true, :user_name=>"postmaster@mailgun.example.com", :password=>"12d3f673e0a83c97045eb4e2d10ebc8a4"}}

Source yaml.

development:
  smtp_settings:
    address: "smtp.mailgun.org"
    port: 587
    domain: "example.com"
    authentication: "plain"
    enable_starttls_auto: true
    user_name: "postmaster@mailgun.example.com"
    password: "12d3f673e0a83c97045eb4e2d10ebc8a4"

Benchmarks

To do benchmarks I wrote a rake task which loaded the environment, printed the secrets and exited.

With symbolize_keys:
2.36 real 1.94 user 0.55 sys
2.38 real 1.93 user 0.51 sys

With deep_symbolize_keys:
2.37 real 1.91 user 0.51 sys
2.31 real 1.89 user 0.49 sys

As you can see there doesn't seem to be a negative effect in load time for this change. I wasn't able to find where the secrets.yml loading was being tested, but if someone could point that out to me I'll add tests as well.

@rails-bot

This comment has been minimized.

Show comment
Hide comment
@rails-bot

rails-bot Oct 29, 2016

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @sgrif (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review.

Please see the contribution instructions for more information.

rails-bot commented Oct 29, 2016

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @sgrif (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review.

Please see the contribution instructions for more information.

@maclover7 maclover7 added the railties label Oct 29, 2016

@sgrif

This comment has been minimized.

Show comment
Hide comment
@sgrif

sgrif Nov 1, 2016

Member

Can you add a test case for this?

Member

sgrif commented Nov 1, 2016

Can you add a test case for this?

@elorest

This comment has been minimized.

Show comment
Hide comment
@elorest

elorest Nov 1, 2016

Contributor

@sgrif I would love too. Could you point me to where tests for secret.yml loading are currently being done? Would this be the correct location? https://github.com/rails/rails/blob/master/railties/test/application/configuration_test.rb#L446

Contributor

elorest commented Nov 1, 2016

@sgrif I would love too. Could you point me to where tests for secret.yml loading are currently being done? Would this be the correct location? https://github.com/rails/rails/blob/master/railties/test/application/configuration_test.rb#L446

@sgrif

This comment has been minimized.

Show comment
Hide comment
@sgrif

sgrif Nov 1, 2016

Member

Yes, those tests which are creating .secrets.yml and asserting the result seems like a fine place.

Member

sgrif commented Nov 1, 2016

Yes, those tests which are creating .secrets.yml and asserting the result seems like a fine place.

@elorest

This comment has been minimized.

Show comment
Hide comment
@elorest

elorest Nov 1, 2016

Contributor

Thanks. I'll add those after after work today.

Contributor

elorest commented Nov 1, 2016

Thanks. I'll add those after after work today.

@maclover7 maclover7 added the needs work label Nov 2, 2016

@elorest

This comment has been minimized.

Show comment
Hide comment
@elorest

elorest Nov 2, 2016

Contributor

Tests have been added. Other than just consistency I feel deep serializing works better because it allows you to setup configurations with many options by just passing the hash in. For instance
config.action_mailer.smtp_settings = Rails.application.secrets.smtp_settings in application.rb only works if keys are symbols.

Contributor

elorest commented Nov 2, 2016

Tests have been added. Other than just consistency I feel deep serializing works better because it allows you to setup configurations with many options by just passing the hash in. For instance
config.action_mailer.smtp_settings = Rails.application.secrets.smtp_settings in application.rb only works if keys are symbols.

@rafaelfranca

Could you add a CHANGELOG entry and squash your commits? Even that is small it is a breaking change.

@elorest

This comment has been minimized.

Show comment
Hide comment
@elorest

elorest Nov 21, 2016

Contributor

@maclover7 I did as you requested but got an conflict on CHANGELOG.md so I merged in master but can't seem to squash the merge. Also weirdly after I merged in master, travis checks aren't passing... Any suggestions?

Contributor

elorest commented Nov 21, 2016

@maclover7 I did as you requested but got an conflict on CHANGELOG.md so I merged in master but can't seem to squash the merge. Also weirdly after I merged in master, travis checks aren't passing... Any suggestions?

@maclover7

This comment has been minimized.

Show comment
Hide comment
@maclover7

maclover7 Nov 21, 2016

Member

@elorest hmm, do you have the latest master locally? I believe you should be able to squash down to one commit via git rebase -i master

Member

maclover7 commented Nov 21, 2016

@elorest hmm, do you have the latest master locally? I believe you should be able to squash down to one commit via git rebase -i master

@elorest

This comment has been minimized.

Show comment
Hide comment
@elorest

elorest Nov 21, 2016

Contributor

@maclover7 I created this pull request from my branch is_deep_symbolize_keys. rebase -i isn't working like normal. It brings up thousands of commits. So I just switch back to master, cherry-picked my commit and then force pushed it back to my is_deep_symbolize_secrets on github. Should be good now.

Contributor

elorest commented Nov 21, 2016

@maclover7 I created this pull request from my branch is_deep_symbolize_keys. rebase -i isn't working like normal. It brings up thousands of commits. So I just switch back to master, cherry-picked my commit and then force pushed it back to my is_deep_symbolize_secrets on github. Should be good now.

@elorest

This comment has been minimized.

Show comment
Hide comment
@elorest

elorest Nov 21, 2016

Contributor

@maclover7 Any idea why travis doesn't pass since I merged in master?

Contributor

elorest commented Nov 21, 2016

@maclover7 Any idea why travis doesn't pass since I merged in master?

@guilleiguaran

This comment has been minimized.

Show comment
Hide comment
@guilleiguaran

guilleiguaran Nov 21, 2016

Member

Failures in Travis aren't related to this PR

Member

guilleiguaran commented Nov 21, 2016

Failures in Travis aren't related to this PR

@guilleiguaran guilleiguaran merged commit 6cd6586 into rails:master Nov 21, 2016

0 of 2 checks passed

codeclimate Code Climate is analyzing this code.
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@guilleiguaran

This comment has been minimized.

Show comment
Hide comment
@guilleiguaran

guilleiguaran Nov 21, 2016

Member

Thank you very much for this 👏

Member

guilleiguaran commented Nov 21, 2016

Thank you very much for this 👏

@maclover7 maclover7 removed the needs work label Nov 22, 2016

@kirs

This comment has been minimized.

Show comment
Hide comment
@kirs

kirs Nov 22, 2016

Member

Does this preserve backward compatibility if I usedapp.secrets.smtp_settings['password']? Looks like it doesn't 😬
Maybe it's worth using HashWithIndifferentAccess here?

Member

kirs commented Nov 22, 2016

Does this preserve backward compatibility if I usedapp.secrets.smtp_settings['password']? Looks like it doesn't 😬
Maybe it's worth using HashWithIndifferentAccess here?

@guilleiguaran

This comment has been minimized.

Show comment
Hide comment
@guilleiguaran

guilleiguaran Nov 22, 2016

Member

@kirs: yes, absolutely makes sense

Member

guilleiguaran commented Nov 22, 2016

@kirs: yes, absolutely makes sense

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Nov 22, 2016

Member

I don't think it is needed. Like I said in the comment above it is fine to have this small backwards incompatible change as it is in the CHANGELOG.

Member

rafaelfranca commented Nov 22, 2016

I don't think it is needed. Like I said in the comment above it is fine to have this small backwards incompatible change as it is in the CHANGELOG.

@elorest

This comment has been minimized.

Show comment
Hide comment
@elorest

elorest Nov 22, 2016

Contributor

I considered doing that, but using HashWithIndifferentAccess did slow down my benchmark of loading and printing by a small amount. @kirs Default smtp settings for rails expect symbols when you mass assign them so config.smtp_settings = app.secrets.smtp_settings is now possible.

Contributor

elorest commented Nov 22, 2016

I considered doing that, but using HashWithIndifferentAccess did slow down my benchmark of loading and printing by a small amount. @kirs Default smtp settings for rails expect symbols when you mass assign them so config.smtp_settings = app.secrets.smtp_settings is now possible.

@printercu

This comment has been minimized.

Show comment
Hide comment
@printercu

printercu Apr 21, 2017

Contributor

@elorest @rafaelfranca This change breaks some gems. There is much more_indifferent access_ on every request when accessing params, don't think this would be a problem to make secrets work the same way.

Contributor

printercu commented Apr 21, 2017

@elorest @rafaelfranca This change breaks some gems. There is much more_indifferent access_ on every request when accessing params, don't think this would be a problem to make secrets work the same way.

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Apr 21, 2017

Member

Added a note in the upgrade guide 0541a0d

Member

rafaelfranca commented Apr 21, 2017

Added a note in the upgrade guide 0541a0d

rafaelfranca added a commit that referenced this pull request Apr 21, 2017

Add note about #26929 in the upgrade guide
This is a small breaking change that we chose to make in 5.1 since the
fix can be done with a search and replace tool.

rafaelfranca added a commit that referenced this pull request Apr 21, 2017

Add note about #26929 in the upgrade guide
This is a small breaking change that we chose to make in 5.1 since the
fix can be done with a search and replace tool.

abicky added a commit to abicky/yaml_vault that referenced this pull request Dec 15, 2017

Support Rails 5.1
In Rails 5.1, nested keys are also symbolized
cf. rails/rails#26929

abicky added a commit to abicky/yaml_vault that referenced this pull request Dec 15, 2017

Support Rails 5.1
In Rails 5.1, nested keys are also symbolized
cf. rails/rails#26929

abicky added a commit to abicky/yaml_vault that referenced this pull request Dec 15, 2017

Support Rails 5.1
In Rails 5.1, nested keys are also symbolized
cf. rails/rails#26929

@abicky abicky referenced this pull request Dec 15, 2017

Merged

Support Rails 5.1 #13

abicky added a commit to abicky/yaml_vault that referenced this pull request Dec 15, 2017

Support Rails 5.1
In Rails 5.1, nested keys are also symbolized
cf. rails/rails#26929
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment