Migrate to Credentials + Remove Other Deprecation Warnings #1326
Conversation
kartiki975
changed the title
|
Just to check - were we able to test these changes in our Shipit instance? |
|
@kwboyd-shopify I didn't do a tophat but the CI checks were passing when I temporarily integrated this branch's changes here: https://github.com/Shopify/shipit/pull/2589/commits/bd719ee152153205b97bdd1d47279ef7a9a7c124 |
|
Gotcha! I'll pull these changes and do a quick test today before approving. |
|
With a recent tophat on internal Shipit with these changes, a 500 error is preventing batched changes to start deploying. No longer looking for reviews until I fix that issue 😞 |
| @@ -8,7 +8,7 @@ class ApiClient < Record | |||
|
|
|||
| validates :creator, :name, presence: true | |||
|
|
|||
| serialize :permissions, Shipit.serialized_column(:permissions, type: Array) | |||
| serialize :permissions, coder: Shipit.serialized_column(:permissions, type: Array) | |||
There was a problem hiding this comment.
I'm not sure what the coder key does; could you explain it for me? 😅 My gut says this overrides default YAML behavior with whatever method you pass, but I wanted to make sure.
There was a problem hiding this comment.
|
Thanks for tophatting as well, @erik-shopify! |
CHANGELOG.md
Outdated
| @@ -1,4 +1,7 @@ | |||
| # Unreleased | |||
| * Migrate secrets from credentials | |||
There was a problem hiding this comment.
Do we have a migration path we can suggest to folks?
| * Migrate secrets from credentials | |
| * Migrate from legacy Rails secrets to credentials | |
| - Rails secrets were [deprecated in Rails 7.1](https://github.com/rails/rails/pull/48472) |
There was a problem hiding this comment.
There is a bunch of blog posts out there, I haven't checked how good they were but at least linking to the guide on credential would be a good start: https://guides.rubyonrails.org/security.html#custom-credentials
There was a problem hiding this comment.
I was looking at that one, and that'd definitely be useful to link, but I was thinking specifically along the lines of a page that gives the steps to migrate from one to the other.
There was a problem hiding this comment.
This was the one that was recommended in discourse: https://github.com/Shopify/ejson-rails?tab=readme-ov-file#migrating-to-credentials.
There was a problem hiding this comment.
Thanks for the suggestions! I have incorporated it all. Let me know if there are any other tweaks needed.
CHANGELOG.md
Outdated
| * Migrate from legacy Rails secrets to credentials (#1326) | ||
| * Rails secrets were [deprecated in Rails 7.1](https://github.com/rails/rails/pull/48472) | ||
| * [Guide on credentials](https://guides.rubyonrails.org/security.html#custom-credentials) | ||
| * Migrated using the ejson-rails gem and [these migration instructions](https://github.com/Shopify/ejson-rails?tab=readme-ov-file#migrating-to-credentials) |
There was a problem hiding this comment.
ejson-rails only concerns Shopify. It's a public gem so people can use it if they way, but as far as shipit-engine (the open source project) is concerned, it only migrated to Rails credentials, not to ejson-rails.
There was a problem hiding this comment.
Unsure if this implied rewording this or removing the line completely. I assumed the latter and removed it in the recent commit.
Create secrets.test.json Replace deprecated Rails.application.secrets Add ejson-rails gem Remove deprecation warning Passing the coder as positional argument is deprecated and will be removed in Rails 7.2 Remove deprecated TestFixtures.fixture_path= Remove deprecated check_pending!
kartiki975
force-pushed
the
remove-deprecation-warning
branch
from
4fb7f33
to
9034b7b
Compare
In attempts to use the recent shipit-engine release, I noticed the following exception being thrown in CI checks:
This PR seeks to avoid this exception by migrating to credentials as instructed here: https://github.com/Shopify/ejson-rails?tab=readme-ov-file#migrating-to-credentials.
There were 3 other deprecation warnings that were being reported during unit tests. Those have been addressed as well.