Fixes #37517 - Upgrade theforeman-rubocop gem to the v0.1.0

[archanaserver]

What are the changes introduced in this pull request?

• Upgrade theforeman-rubocop gem to version 0.1.0, https://community.theforeman.org/t/standardizing-rubocop-with-theforeman-rubocop/37239/1#use-theforeman-rubocop-010-in-foreman-and-all-plugins-1
• Changed the RuboCop configuration to inherit from the lenient.yml style provided by theforeman-rubocop, https://github.com/theforeman/theforeman-rubocop?tab=readme-ov-file#not-intrusive-style---lenient
• Removed Rails and Style/Documentation cops from the configuration as they are already included in the lenient.yml style.

Considerations taken when implementing this change?

• Chose lenient.yml because it aligns well with the existing RuboCop style followed by this repo.
• Removed Rails and Style/Documentation cops to avoid redundancy and conflicts since they are already covered by lenient.yml.

What are the testing steps for this pull request?

1. Run bundle install to ensure all dependencies are correctly installed, including the updated theforeman-rubocop gem.
2. Execute bundle exec rubocop to verify that the new RuboCop configuration is being applied without errors.
3. Review the output to ensure there are no unexpected offenses or configuration issues.

[archanaserver]

Note: While testing, there are new RuboCop offenses now. It's unclear for me to know which offenses to fix based on the code style, so should I consider regenerating the .rubocop_todo.yml file or I believe we can discuss further.

[jeremylenz]

Here are my thoughts about specific rules that are failing currently:

Style/OptionalBooleanParameter: Prefer keyword arguments for arguments with a boolean default value; use `redirect_on_failure: true` instead of `redirect_on_failure = true`.

Yeah kwargs! 👍 I like them in general because they're more explicit, but they can make function definitions long. In the case of Booleans, though, I think requiring them outweighs that, so I'm for it.

Style/TrailingCommaInHashLiteral: Put a comma after the last item of a multiline hash.

This will be a big change for Katello's Ruby, but it will also bring us in line with eslint and our JS code. If Foreman is adopting this, I think Katello should too. 👍

Style/SlicingWithRange: Prefer ary[n..] over ary[n..-1].

uhh sure why not!

Rails/HttpStatus: Prefer `:ok` over `200` to define HTTP status code.

We do this in most every place, so I'm +1 to this rule.

Style/CaseLikeIf: Convert `if-elsif` to `case-when`.

I do like case-when better, but this may get pushback from some veteran Rubyists. @parthaa thoughts?

[chris1984]

Here are my thoughts about specific rules that are failing currently:

Style/OptionalBooleanParameter: Prefer keyword arguments for arguments with a boolean default value; use `redirect_on_failure: true` instead of `redirect_on_failure = true`.

Yeah kwargs! 👍 I like them in general because they're more explicit, but they can make function definitions long. In the case of Booleans, though, I think requiring them outweighs that, so I'm for it.

Style/TrailingCommaInHashLiteral: Put a comma after the last item of a multiline hash.

This will be a big change for Katello's Ruby, but it will also bring us in line with eslint and our JS code. If Foreman is adopting this, I think Katello should too. 👍

Style/SlicingWithRange: Prefer ary[n..] over ary[n..-1].

uhh sure why not!

Rails/HttpStatus: Prefer `:ok` over `200` to define HTTP status code.

We do this in most every place, so I'm +1 to this rule.

Style/CaseLikeIf: Convert `if-elsif` to `case-when`.

I do like case-when better, but this may get pushback from some veteran Rubyists. @parthaa thoughts?

I'm a junior Rubyist if that counts for anything?

[jeremylenz]

@archanaserver I haven't seen any dissents so I'd say you can go ahead with keeping these rules and fixing them.

[archanaserver]

Style/OptionalBooleanParameter: Prefer keyword arguments for arguments with a boolean default value; use `redirect_on_failure: true` instead of `redirect_on_failure = true`.

Yeah kwargs! 👍 I like them in general because they're more explicit, but they can make function definitions long. In the case of Booleans, though, I think requiring them outweighs that, so I'm for it.

@jeremylenz yes, so looking at the Style/OptionalBooleanParameter rule causing approx 60 offenses rn, changing to boolean keyword arguments may enhance our code readability and clarity, but implementing such changes should be considered in a huge refactor. This change would require every method call across the codebase, and could break things. I guess just to make RuboCop happy, this would not be a good idea.

Out current implementation works correctly here and this rule is purely a stylistic preference only and no any other concern. Therefore, I recommend not enforcing this rule at this time to avoid unnecessary complications. We have followed the same approach in some other repos. WDYT?

Also, I have raised few PRs for different cops. Do you want me to update the rubocop_todo file after the changes got merged?

[jeremylenz]

I recommend not enforcing this rule at this time to avoid unnecessary complications. We have followed the same approach in some other repos.

If we've got consistency across repos, I'm fine with this. 👍

I have raised few PRs for different cops. Do you want me to update the rubocop_todo file after the changes got merged?

What does that mean exactly?

[archanaserver]

I have raised few PRs for different cops. Do you want me to update the rubocop_todo file after the changes got merged?

What does that mean exactly?

@jeremylenz so even after fixing cops like Style/TrailingCommaInHashLiteral, Style/SlicingWithRange, Rails/HttpStatus and Style/CaseLikeIf. There are still many other cops causing failure in RuboCop job run.

Some of them are from Style/, Layout/, Lint/, Metrics/ and Minitest/. Do you want continue fixing each one of them or want to proceed for now by updating .rubocop_todo.yml file, adding all those in todo list for later fix.

[jeremylenz]

Do you want continue fixing each one of them or want to proceed for now by updating .rubocop_todo.yml file, adding all those in todo list for later fix.

I would prefer to have little or nothing in todo. I feel like most of them should be resolved with --auto-correct? How does it look after you run that?

[archanaserver]

This is to track list of PRs submitted to fixing cops, after the theforeman-rubocop version update:

• Fix cop Style/TrailingCommaInHashLiteral #11043
• Fix Rails/HttpStatus cop #11046
• Fix Style/CaseLikeIf cop #11047
• Fix Style/SlicingWithRange cop #11048
• Fix some cops #11064
• Fix some Minitest/ cops #11065
• Fix Style/SoleNestedConditional cop #11066
• Fix Style Cops #11067
• Fix layout cops #11068
• Fix Various Lint Cops  #11069

[archanaserver]

Do you want continue fixing each one of them or want to proceed for now by updating .rubocop_todo.yml file, adding all those in todo list for later fix.

I would prefer to have little or nothing in todo. I feel like most of them should be resolved with --auto-correct? How does it look after you run that?

@jeremylenz I'm on it! I did use rubocop --auto-correct, but some offenses required manually adjustments or a scan through to ensure that the change not breaking anything.

I'm making sure to fix the rules which seems relevant and safe to use, or decided to put some marked disabled by following rest of the foreman repo's, like this 9ab6e28. WDYT?

[jeremylenz]

I'm making sure to fix the rules which seems relevant and safe to use, or decided to put some marked disabled by following rest of the foreman repo's, like this 9ab6e28. WDYT?

If we are consistent with other repos then 👍 🚀

[ianballou]

@archanaserver is there a particular order that you'd like these PRs merged? I wasn't sure if there were interdependencies that required some to go before others.
To avoid having a period of time where rubocop is broken across the board, I guess we should merge all of them at once?

[archanaserver]

@archanaserver is there a particular order that you'd like these PRs merged? I wasn't sure if there were interdependencies that required some to go before others. To avoid having a period of time where rubocop is broken across the board, I guess we should merge all of them at once?

@ianballou This particular PR require all of these PRs to merge(in any order) first, so that we have all the new rubocop rules here after the theforeman-rubocop gem update to v0.1.0. Also this will make the rubocop job green here.

[archanaserver]

@ianballou some of the PRs from the list are approved by others and ready to be merge i belive, would you mind taking a look?

[archanaserver]

@ianballou i don't understand this failure, can you help me here? As per the code change I have specified this gem only once, not sure from where it is detecting two different versions.

 [!] There was an error parsing `Gemfile`: You cannot specify the same gem twice with different version requirements.
You specified: theforeman-rubocop (~> 0.0.6) and theforeman-rubocop (~> 0.1.0). Bundler cannot continue.

[archanaserver]

@ianballou i don't understand this failure, can you help me here? As per the code change I have specified this gem only once, not sure from where it is detecting two different versions.

 [!] There was an error parsing `Gemfile`: You cannot specify the same gem twice with different version requirements.
You specified: theforeman-rubocop (~> 0.0.6) and theforeman-rubocop (~> 0.1.0). Bundler cannot continue.

i've rebase this with recent changes but seeing the above issue again, if someone can point me here what i am missing here?

[ianballou]

I'm out for a while, so let me redirect this to @sjha4 or @qcjames53

[archanaserver]

hie @ianballou, could you please find moment to dropping some reviews here?

[ianballou]

Looking pretty good overall! Small amount of failures left after rebasing.

Since Foreman needs to update their version of theforeman-rubocop, is the plan to merge a Foreman PR right after the Katello one is merged? Otherwise our tests will be broken.

Here's the latest output from rubocop after rebasing:

/home/vagrant/katello/app/controllers/katello/api/v2/activation_keys_controller.rb:303:7: C: Naming/MemoizedInstanceVariableName: Memoized variable @organization does not match method name find_content_view_environments. Use @find_content_view_environments instead.
      @organization ||= @content_view_environments.first&.organization
      ^^^^^^^^^^^^^
/home/vagrant/katello/app/models/katello/content_view_environment.rb:71:7: C: [Correctable] Style/CaseLikeIf: Convert if-elsif to case-when.
      if content_object.is_a? Katello::ActivationKey ...
      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
/home/vagrant/katello/app/models/katello/content_view_environment.rb:78:46: C: [Correctable] Style/KeywordParametersOrder: Place optional keyword parameters at the end of the parameters list.
    def self.fetch_content_view_environments(labels: [], ids: [], organization:)
                                             ^^^^^^^^^^
/home/vagrant/katello/app/models/katello/content_view_environment.rb:78:58: C: Style/KeywordParametersOrder: Place optional keyword parameters at the end of the parameters list.
    def self.fetch_content_view_environments(labels: [], ids: [], organization:)
                                                         ^^^^^^^
/home/vagrant/katello/engines/bastion/bastion.gemspec:1:22: C: [Correctable] Style/ExpandPathArguments: Use expand_path('lib', __dir__) instead of expand_path('../lib', __FILE__).
$LOAD_PATH.push File.expand_path("../lib", __FILE__)
                     ^^^^^^^^^^^
/home/vagrant/katello/engines/bastion/bastion.gemspec:14:33: C: Gemspec/RequiredRubyVersion: required_ruby_version and TargetRubyVersion (2.7, which may be specified in .rubocop.yml) should be equal.
  s.gem.required_ruby_version = ['>= 2.5.0', '< 2.7.0']
                                ^^^^^^^^^^^^^^^^^^^^^^^
/home/vagrant/katello/katello.gemspec:1:22: C: [Correctable] Style/ExpandPathArguments: Use expand_path('lib', __dir__) instead of expand_path('../lib', __FILE__).
$LOAD_PATH.push File.expand_path("../lib", __FILE__)
                     ^^^^^^^^^^^
/home/vagrant/katello/lib/katello/permission_creator.rb:56:28: C: [Correctable] Style/TrailingCommaInHashLiteral: Put a comma after the last item of a multiline hash.
                           'katello/api/v2/capsules' => [:index, :show]
                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
/home/vagrant/katello/test/models/association_test.rb:70:9: C: [Correctable] Style/TrailingCommaInArrayLiteral: Put a comma after the last item of a multiline array.
        "Katello::SubscriptionFacetPurposeAddon"
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
/home/vagrant/katello/test/models/content_view_environment_activation_key_test.rb:18:73: C: [Correctable] Layout/MultilineArrayBraceLayout: The closing array brace must be on the line after the last array element when the opening brace is on a separate line from the first array element.
        katello_content_view_environments(:library_dev_staging_view_dev)]
                                                                        ^

1932 files inspected, 10 offenses detected, 7 offenses auto-correctable

[ianballou]

Since the tests are broken I'm running them locally. Will report back soon.

[ianballou]

Only saw one test failure that is likely not related to this PR:

Error:
Katello::PxeFilesDownloaderTest#test_fetches_pxe_files:
OpenSSL::X509::CertificateError: invalid digest
    /home/vagrant/katello/test/services/katello/pxe_files_downloader_test.rb:49:in `sign'
    /home/vagrant/katello/test/services/katello/pxe_files_downloader_test.rb:49:in `generate_certificate'
    /home/vagrant/katello/test/services/katello/pxe_files_downloader_test.rb:14:in `test_fetches_pxe_files'

[ianballou]

This should be good to ACK once the rebase is done 👍

[archanaserver]

Thanks @ianballou! I just rebased it and solved some of the cops offenses.

Since Foreman needs to update their version of theforeman-rubocop, is the plan to merge a Foreman PR right after the Katello one is merged? Otherwise our tests will be broken.

And about this, I guess mentioning @ekohl would be good for a clear answer.

[ehelms]

[!] There was an error parsing Gemfile: You cannot specify the same gem twice with different version requirements.
You specified: theforeman-rubocop (> 0.0.6) and theforeman-rubocop (> 0.1.0). Bundler cannot continue.

I see this error output. Is this due to Katello specifying theforeman-rubocop and inheriting it via Foreman? If katello is inheriting it from Foreman, why specify it in the test.rb Gemfile?

I think, unlike other plugins, Katello has a tight relationship with Foreman's target version of the theforeman-rubocop that creates this coupling and this will happen anytime the version gets bumped by one or the other.

I am thinking one of two approaches would help:

1. Move theforeman-rubocop to the .gemspec as a development dependency like other plugins (https://github.com/theforeman/foreman_ansible/blob/master/foreman_ansible.gemspec#L26). This will mean when the test suite is run it runs with --without development and it will ignore this. But it will grab it when it does the rubocop job.

2. Add a gemfile.d/lint.rb file and specify theforeman-rubocop there, and then make sure the test suite includes --without lint

One of the above changes can be made as an independent PR.

[ianballou]

I think, unlike other plugins, Katello has a tight relationship with Foreman's target version of the theforeman-rubocop that creates this coupling and this will happen anytime the version gets bumped by one or the other.

Any chance this is just leftover from when we ran rubocop directly in Katello instead of via foreman-rake katello:rubocop? I wonder if it can be removed entirely.

[archanaserver]

I think, unlike other plugins, Katello has a tight relationship with Foreman's target version of the theforeman-rubocop that creates this coupling and this will happen anytime the version gets bumped by one or the other.

Any chance this is just leftover from when we ran rubocop directly in Katello instead of via foreman-rake katello:rubocop? I wonder if it can be removed entirely.

I believe in that case it’s better to first test removing the gem declaration from Katello's Gemfile and rely on Foreman's version to see if this resolves the conflict. That would also mean we need to merge the Foreman PR first. If that doesn’t work, I’ll explore the alternative options that @ehelms suggested, such as moving it to the .gemspec or gemfile.d/lint.rb.