Migrate to Credentials + Remove Other Deprecation Warnings #1326

kartiki975 · 2024-01-30T20:56:16Z

In attempts to use the recent shipit-engine release, I noticed the following exception being thrown in CI checks:

ActiveSupport::DeprecationException: DEPRECATION WARNING: `Rails.application.secrets` is deprecated in favor of `Rails.application.credentials` and will be removed in Rails 7.2. (called from block in <class:Engine> at /tmp/bundle/ruby/3.2.0/bundler/gems/shipit-engine-03a10e90a389/[redacted]) (ActiveSupport::DeprecationException)

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.

kwboyd-shopify · 2024-01-30T22:05:24Z

Just to check - were we able to test these changes in our Shipit instance?

kartiki975 · 2024-01-31T02:27:16Z

@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

kwboyd-shopify · 2024-01-31T16:56:16Z

Gotcha! I'll pull these changes and do a quick test today before approving.

kartiki975 · 2024-02-05T19:19:44Z

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 😞

emmabukacek

I'd feel better if we had at least one more review on this, but I read through the migration guide and this, and nothing stood out to me as incorrect! Thanks for taking on this massive effort. 🙏🏻

emmabukacek · 2024-02-06T18:14:18Z

app/models/shipit/api_client.rb

@@ -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)


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.

Yes: https://api.rubyonrails.org/classes/ActiveRecord/AttributeMethods/Serialization/ClassMethods.html#method-i-serialize

lib/shipit.rb

kartiki975 · 2024-02-07T16:14:41Z

Thanks for tophatting as well, @erik-shopify!
In regards to indefinite soaking, I am not seeing that on my end... that may be flakiness 🤔 let me know if you see anything unusual on any of your recent tophat attempts.

CHANGELOG.md

jgaskins · 2024-02-07T17:00:44Z

CHANGELOG.md

@@ -1,4 +1,7 @@
 # Unreleased
+* Migrate secrets from credentials


Do we have a migration path we can suggest to folks?

Suggested change

* 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 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

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.

This was the one that was recommended in discourse: https://github.com/Shopify/ejson-rails?tab=readme-ov-file#migrating-to-credentials.

Thanks for the suggestions! I have incorporated it all. Let me know if there are any other tweaks needed.

ruionweb

If you also don't mind including the octokit bump in the changelog! #1327

casperisfine · 2024-02-07T18:04:08Z

CHANGELOG.md

+* 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)


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.

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 changed the title ~~Remove deprecation warning~~ Remove deprecation warnings Jan 30, 2024

kartiki975 changed the title ~~Remove deprecation warnings~~ Migrate to Credentials + Remove Other Deprecation Warnings Jan 30, 2024

kartiki975 requested a review from a team January 30, 2024 21:22

kartiki975 marked this pull request as ready for review January 30, 2024 21:22

emmabukacek approved these changes Feb 6, 2024

View reviewed changes

casperisfine reviewed Feb 7, 2024

View reviewed changes

lib/shipit.rb Show resolved Hide resolved

kartiki975 requested review from a team and casperisfine February 7, 2024 16:16

kwboyd-shopify approved these changes Feb 7, 2024

View reviewed changes

casperisfine reviewed Feb 7, 2024

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

jgaskins reviewed Feb 7, 2024

View reviewed changes

ruionweb approved these changes Feb 7, 2024

View reviewed changes

erik-shopify approved these changes Feb 7, 2024

View reviewed changes

kartiki975 requested review from casperisfine and jgaskins February 7, 2024 17:41

casperisfine reviewed Feb 7, 2024

View reviewed changes

kartiki975 force-pushed the remove-deprecation-warning branch from 4fb7f33 to 9034b7b Compare February 7, 2024 19:17

kartiki975 merged commit d5cf443 into main Feb 7, 2024
17 checks passed

kartiki975 deleted the remove-deprecation-warning branch February 7, 2024 21:01

shopify-shipit bot temporarily deployed to rubygems February 27, 2024 21:45 Inactive

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate to Credentials + Remove Other Deprecation Warnings #1326

Migrate to Credentials + Remove Other Deprecation Warnings #1326

kartiki975 commented Jan 30, 2024 •

edited

kwboyd-shopify commented Jan 30, 2024

kartiki975 commented Jan 31, 2024

kwboyd-shopify commented Jan 31, 2024

kartiki975 commented Feb 5, 2024

emmabukacek left a comment

emmabukacek Feb 6, 2024

casperisfine Feb 7, 2024

emmabukacek Feb 7, 2024

kartiki975 commented Feb 7, 2024

jgaskins Feb 7, 2024

casperisfine Feb 7, 2024

jgaskins Feb 7, 2024

kartiki975 Feb 7, 2024

kartiki975 Feb 7, 2024

ruionweb left a comment

casperisfine Feb 7, 2024

kartiki975 Feb 7, 2024

		@@ -1,4 +1,7 @@
		# Unreleased
		* Migrate secrets from credentials

Migrate to Credentials + Remove Other Deprecation Warnings #1326

Migrate to Credentials + Remove Other Deprecation Warnings #1326

Conversation

kartiki975 commented Jan 30, 2024 • edited

kwboyd-shopify commented Jan 30, 2024

kartiki975 commented Jan 31, 2024

kwboyd-shopify commented Jan 31, 2024

kartiki975 commented Feb 5, 2024

emmabukacek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kartiki975 commented Feb 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ruionweb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kartiki975 commented Jan 30, 2024 •

edited