Fixes #37852 - Support Rails 7.0 #11155

ofedoren · 2024-09-20T15:00:05Z

What are the changes introduced in this pull request?

We're updating Rails version in Foreman: theforeman/foreman#10299 (please visit it first if you have any questions/suggestions, there are many comments regarding the changes)

This PR tries to make Katello Rails 7 friendly (compatible).

Considerations taken when implementing this change?

Mostly removing deprecated usage of #<< for ActiveModel::Errors objects
Just a heads up regarding https://guides.rubyonrails.org/upgrading_ruby_on_rails.html#digest-class-for-activesupport-digest-changing-to-sha256. How it affects this plugin is more or less here: Fixes #37825 - Upgrade Rails to 7.0 theforeman/foreman#10299 (comment)

What are the testing steps for this pull request?

Ensuring CI is green.

If you really want some 🍪, test some workflows live.

ekohl

I see lots of changes around the error handling and I think that's already Rails 6.1 compatible. Perhaps something that we can already merge to reduce the size of this PR?

ekohl · 2024-10-22T14:57:55Z

test/lib/validators/hostgroup_kickstart_repository_validator_test.rb

@@ -63,7 +63,7 @@ def setup

      @validator.validate(@content_facet)

-      assert_empty @content_facet.errors.values
+      assert_empty @content_facet.errors.map { |error| error.message }


Suggested change

assert_empty @content_facet.errors.map { |error| error.message }

assert_empty @content_facet.errors.map(&:message)

chris1984 · 2024-10-23T17:57:45Z

@SatelliteQE/TeamPhoenix

Satellite-QE · 2024-10-24T09:37:17Z

PRT Result

Build Number: 9132
Build Status: UNSTABLE
PRT Comment: pytest tests/foreman --team Phoenix-content --external-logging
Test Result: = 6 skipped, 3973 deselected, 5492 warnings, 1490 errors in 4906.50s (1:21:46) =

Satellite-QE · 2024-10-24T09:43:15Z

PRT Result

Build Number: 9133
Build Status: UNSTABLE
PRT Comment: pytest tests/foreman --team Phoenix-subscriptions --external-logging
Test Result: ======= 4779 deselected, 5492 warnings, 690 errors in 4923.46s (1:22:03) =======

ekohl · 2024-10-24T11:00:18Z

@chris1984 I doubt PRT is going to be of any use. You need updated Foreman and Rails and we haven't packaged that.

ofedoren · 2024-11-07T15:14:46Z

The test failure is unrelated to this PR, it for some reason wasn't caught by GHA for theforeman/foreman_remote_execution#913

Fixed in theforeman/foreman_remote_execution#925

ofedoren · 2024-11-14T16:14:14Z

app/lib/katello/util/data.rb

@@ -6,7 +6,7 @@ def self.array_with_indifferent_access(variable)
      end

      def self.hexdigest(string)
-        defined?(ActiveSupport::Digest) ? ActiveSupport::Digest.hexdigest(string) : Digest::MD5.hexdigest(string)
+        defined?(ActiveSupport::Digest) ? OpenSSL::Digest::SHA1.hexdigest(string)[0...32] : Digest::MD5.hexdigest(string)


This forces SHA1 for hexdigest since it was default in Rails < 7. This makes sure Katello behaves the same way and the tests are green.

I'd ask maintainers to check the needed changes for this plugin to support SHA256. Afterwards this line can be switched back (until Rails upgrades the default to SHA512).

@parthaa said in https://community.theforeman.org/t/rails-7-upgrade/37771/27 that the actual value doesn't matter, as long as it's consistent.

I wonder if the old code was ever used. rails/rails@82822a3 introduced ActiveSupport::Digest and was introduced in Rails 5.2 (by a former Foreman developer). Perhaps a PR to replace it al with direct calls to ActiveSupport::Digest.hexdigest() is a better approach.

Having said that, I find it interesting that the class itself still defaults to md5: https://github.com/rails/rails/blob/4df235f7c0149ac7be86580d41a74a7785cb9bb9/activesupport/lib/active_support/digest.rb#L9

So should I just revert the changes back and simply update IDs in VCR tests?

Since the IDs just need to be random numbers, I think it's fine to use the new SHA 256 default. I was going to suggest continuing to use SHA1 like suggested here so the VCR recordings could be avoided, but that's already done, so I think we're all good.

I just thought if it will break stuff for upgrading users:

IDs are generated or rather mapped (for the same input A -> the same output B)

These IDs are stored somewhere

Newer hashing class will create for the same input A -> the same output, but different C

The stored B can't be referenced by A, since hashed version is C.

But if mapping goes only before storing and then the app using only stored values (without any further #hexdigest calls on A to reference stored value) then yeah, it should be safe. We just would need to update tests whenever new default comes from Rails.

I haven't found anywhere where we actually try to regenerate and look up any IDs that get created via hexdigest. Rather, we work with the value that's stored in the database.

ekohl · 2024-11-19T13:23:56Z

katello.gemspec

@@ -63,7 +63,7 @@ Gem::Specification.new do |gem|

  # UI
  gem.add_dependency "deface", '>= 1.0.2', '< 2.0.0'
-  gem.add_dependency "angular-rails-templates", "~> 1.1.0"
+  gem.add_dependency "angular-rails-templates", "~> 1.2.0"


I split this off into #11222.

ekohl · 2024-11-20T14:31:27Z

I see you split off #11226 into a separate PR, which is great. Are you also planning to split of the removal of require statements? I'd think that's also compatible with our current Zeitwerk and that only scopes this PR to the digest changes.

ianballou · 2024-11-20T21:27:49Z

.github/workflows/ruby.yml

@@ -27,7 +27,7 @@ jobs:
      plugin: katello
      postgresql_container: ghcr.io/theforeman/postgresql-evr
      test_existing_database: false
-      foreman_version: develop # set to the Foreman release branch after branching :)
+      foreman_version: refs/pull/10299/head # set to the Foreman release branch after branching :)


Comment reminder to revert this back to develop before merging.

ekohl · 2024-11-20T21:52:56Z

When I locally rebase this I see there are some changes remaining where the template parameter to respond_for_show and respond_for_index is changed into full_template. Why is that change needed?

parthaa · 2024-11-20T22:27:16Z

When I locally rebase this I see there are some changes remaining where the template parameter to respond_for_show and respond_for_index is changed into full_template. Why is that change needed?

hmm seems connected to

diff --git a/app/lib/katello/api/v2/rendering.rb b/app/lib/katello/api/v2/rendering.rb
index 653e41a76e..a987e9ebde 100644
--- a/app/lib/katello/api/v2/rendering.rb
+++ b/app/lib/katello/api/v2/rendering.rb
@@ -41,8 +41,9 @@ module Katello
         def respond_with_template(action, resource_name, options = {}, &_block)
           yield if block_given?
           status = options[:status] || 200
+          template = options[:full_template] || "katello/api/v2/#{resource_name}/#{action}"

-          render :template => "katello/api/v2/#{resource_name}/#{action}",
+          render :template => template,
                  :status => status,
                  :locals => options.slice(:object_name, :root_name, :locals),
                  :layout => "katello/api/v2/layouts/#{options[:layout]}"

Guessing relative paths dont work with zeitwerk ?

-      respond_for_show(:resource => host, :template => '../../../api/v2/hosts/show')
+      respond_for_show(:resource => host, :full_template => 'katello/api/v2/hosts/show')

ofedoren · 2024-11-21T12:36:31Z

@ekohl, @parthaa, without this change it just stops working on Rails 7. Not sure why, but this change was safe, straightforward and compatible for other places. It just seemed easier to fix it like that than digging to find what change in Rails caused that :/

ianballou

This is looking fine to me, I tested it a bit with theforeman/foreman#10299.
Just don't forget to change the foreman version back to develop for testing!

…HA256

ofedoren · 2024-11-26T12:44:46Z

Sorry for pushing after approval. I've just rebased both Foreman PR and this one, just to ensure it's still green, so I can drop the last commit to make it ready for merge.

ofedoren · 2024-11-26T13:26:37Z

PR is green after rebase, thus dropped the last commit.

pr-processor bot added Not yet reviewed Waiting on contributor labels Sep 20, 2024

github-actions bot added the Packaging Change label Sep 20, 2024

ofedoren force-pushed the feat-37852-support-rails-7 branch from 4f91a1c to 1ac6a68 Compare October 18, 2024 14:00

ekohl reviewed Oct 22, 2024

View reviewed changes

pr-processor bot removed the Not yet reviewed label Oct 22, 2024

ofedoren force-pushed the feat-37852-support-rails-7 branch from 1ac6a68 to b806bb1 Compare November 5, 2024 15:12

pr-processor bot added the Needs re-review label Nov 5, 2024

ofedoren force-pushed the feat-37852-support-rails-7 branch 3 times, most recently from b26b87d to 1f31c0f Compare November 7, 2024 14:27

ofedoren force-pushed the feat-37852-support-rails-7 branch from 1f31c0f to 3ec89a1 Compare November 14, 2024 16:09

ofedoren commented Nov 14, 2024

View reviewed changes

pr-processor bot removed the Needs re-review label Nov 14, 2024

ofedoren force-pushed the feat-37852-support-rails-7 branch from 3ec89a1 to 80bf04c Compare November 14, 2024 16:17

pr-processor bot added Needs re-review and removed Needs re-review labels Nov 14, 2024

ekohl mentioned this pull request Nov 19, 2024

Fixes #38018 - Rails 7 compatible angular-rails-templates #11222

Merged

ekohl reviewed Nov 19, 2024

View reviewed changes

ofedoren force-pushed the feat-37852-support-rails-7 branch from 80bf04c to d05c4ae Compare November 19, 2024 14:11

pr-processor bot added the Needs re-review label Nov 19, 2024

github-actions bot removed the Packaging Change label Nov 19, 2024

ofedoren mentioned this pull request Nov 20, 2024

Fixes #38023 - Stop using deprecated methods for ActiveModel::Errors #11226

Merged

ofedoren mentioned this pull request Nov 20, 2024

Fixes #38026 - Remove redundant 'require' calls #11228

Merged

ofedoren force-pushed the feat-37852-support-rails-7 branch from d05c4ae to b835788 Compare November 20, 2024 17:01

pr-processor bot removed the Needs re-review label Nov 20, 2024

ianballou reviewed Nov 20, 2024

View reviewed changes

ofedoren force-pushed the feat-37852-support-rails-7 branch from b835788 to 622a6dc Compare November 21, 2024 12:27

pr-processor bot added Needs re-review and removed Needs re-review labels Nov 21, 2024

ofedoren force-pushed the feat-37852-support-rails-7 branch from 622a6dc to 4217a15 Compare November 21, 2024 13:14

pr-processor bot added Needs re-review and removed Needs re-review labels Nov 21, 2024

ianballou approved these changes Nov 25, 2024

View reviewed changes

ofedoren added 2 commits November 26, 2024 12:43

Fixes #37852 - Support Rails 7.0

0f87aa6

Refs #37852 - Update VCR cassettes since ActiveSupport::Digest uses S…

a3df767

…HA256

ofedoren force-pushed the feat-37852-support-rails-7 branch from 4217a15 to 928df93 Compare November 26, 2024 12:43

pr-processor bot added the Needs re-review label Nov 26, 2024

ofedoren force-pushed the feat-37852-support-rails-7 branch from 928df93 to a3df767 Compare November 26, 2024 13:26

pr-processor bot removed the Waiting on contributor label Nov 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #37852 - Support Rails 7.0 #11155

Fixes #37852 - Support Rails 7.0 #11155

ofedoren commented Sep 20, 2024 •

edited

Loading

ekohl left a comment

ekohl Oct 22, 2024

chris1984 commented Oct 23, 2024

Satellite-QE commented Oct 24, 2024

Satellite-QE commented Oct 24, 2024

ekohl commented Oct 24, 2024

ofedoren commented Nov 7, 2024 •

edited

Loading

ofedoren Nov 14, 2024

ekohl Nov 18, 2024

ofedoren Nov 19, 2024

ianballou Nov 20, 2024

ofedoren Nov 21, 2024

ianballou Nov 25, 2024

ekohl Nov 19, 2024

ekohl commented Nov 20, 2024

ianballou Nov 20, 2024

ekohl commented Nov 20, 2024

parthaa commented Nov 20, 2024

ofedoren commented Nov 21, 2024 •

edited

Loading

ianballou left a comment

ofedoren commented Nov 26, 2024

ofedoren commented Nov 26, 2024

	assert_empty @content_facet.errors.map { \|error\| error.message }
	assert_empty @content_facet.errors.map(&:message)

Fixes #37852 - Support Rails 7.0 #11155

Are you sure you want to change the base?

Fixes #37852 - Support Rails 7.0 #11155

Conversation

ofedoren commented Sep 20, 2024 • edited Loading

What are the changes introduced in this pull request?

Considerations taken when implementing this change?

What are the testing steps for this pull request?

ekohl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chris1984 commented Oct 23, 2024

Satellite-QE commented Oct 24, 2024

Satellite-QE commented Oct 24, 2024

ekohl commented Oct 24, 2024

ofedoren commented Nov 7, 2024 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ekohl commented Nov 20, 2024

Choose a reason for hiding this comment

ekohl commented Nov 20, 2024

parthaa commented Nov 20, 2024

ofedoren commented Nov 21, 2024 • edited Loading

ianballou left a comment

Choose a reason for hiding this comment

ofedoren commented Nov 26, 2024

ofedoren commented Nov 26, 2024

ofedoren commented Sep 20, 2024 •

edited

Loading

ofedoren commented Nov 7, 2024 •

edited

Loading

ofedoren commented Nov 21, 2024 •

edited

Loading