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

archanaserver · 2024-05-29T14:48:53Z

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?

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

archanaserver · 2024-05-29T14:50:23Z

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 · 2024-06-12T17:31:30Z

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 · 2024-06-20T19:32:30Z

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

archanaserver · 2024-06-25T18:52:52Z

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 · 2024-06-25T18:57:42Z

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 · 2024-06-26T06:33:49Z

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 · 2024-06-26T13:32:26Z

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 · 2024-07-08T09:25:34Z

archanaserver · 2024-07-08T09:32:16Z

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 · 2024-07-09T13:48:40Z

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 · 2024-07-09T19:03:42Z

@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 · 2024-07-11T06:34:46Z

@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 · 2024-07-18T14:11:38Z

@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 · 2024-09-04T07:28:43Z

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 · 2024-09-18T13:26:36Z

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

ianballou · 2024-09-18T13:50:06Z

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 · 2024-09-18T13:57:14Z

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

archanaserver · 2024-09-18T20:28:54Z

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 · 2024-09-19T13:28:05Z

[!] 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:

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.
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 · 2024-09-19T20:27:12Z

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 · 2024-09-19T21:42:18Z

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.

archanaserver · 2024-10-29T09:40:39Z

This now passing all the checks, also added few more fixes for different rubocop offenses. @ianballou @jeremylenz do you have any thoughts on this?

jeremylenz · 2024-10-29T13:29:56Z

If you squash the commits that'll take care of the "Redmine issues" check :)

Choose to inherit `lenient.yml` from `theforeman-rubocop` style, because that it matches to the rubocop style this repo follows. Also dropped `Rails` and `Style/Documentation` cops because that already exist in the lenient style we now follow. Update .rubocop_todo.yml file Fix Style/TrailingCommaInHashLiteral cop Fix cop Style/TrailingCommaInArrayLiteral Fix Style/KeywordParametersOrder cop Fix Style/ClassEqualityComparison cop Fix Style/CommentAnnotation cop Fix Style/RedundantAssignment cop Fix Style/CaseLikeIf cop Fix Lint/AmbiguousBlockAssociation offenses Added parentheses around method calls in assert statements to resolve ambiguous block associations as flagged by RuboCop. Fix Naming/RescuedExceptionsVariableName Fix Style/RegexpLiteral

archanaserver · 2024-11-06T20:59:44Z

If you squash the commits that'll take care of the "Redmine issues" check :)

Done @jeremylenz ! You can take a look now.

ianballou

Can this merge now? Is it waiting on any dependencies?

archanaserver · 2024-11-06T22:02:32Z

Can this merge now? Is it waiting on any dependencies?

Yes, we can merge this now.

ianballou · 2024-11-07T18:39:32Z

Thanks @archanaserver !

pr-processor bot added Not yet reviewed Waiting on contributor labels May 29, 2024

archanaserver force-pushed the rubocop branch from 7a44598 to 3a22ce6 Compare May 29, 2024 14:51

archanaserver changed the title ~~Upgrade theforeman-rubocop gem to the v0.1.0~~ Fixes #37517 - Upgrade theforeman-rubocop gem to the v0.1.0 May 29, 2024

archanaserver force-pushed the rubocop branch from 3a22ce6 to e2f4d65 Compare May 29, 2024 14:58

pr-processor bot removed the Waiting on contributor label May 29, 2024

archanaserver mentioned this pull request May 31, 2024

#1 Utilizing theforeman-rubocop ~> 0.1.0 in Foreman and all Plugins theforeman/theforeman-rubocop#12

Closed

35 tasks

jeremylenz reviewed Jun 12, 2024

View reviewed changes

This was referenced Jun 24, 2024

Fix cop Style/TrailingCommaInHashLiteral #11043

Merged

Fix Rails/HttpStatus cop #11046

Merged

Fix Style/CaseLikeIf cop #11047

Merged

Fix Style/SlicingWithRange cop #11048

Merged

archanaserver mentioned this pull request Jun 28, 2024

Disable Style/OptionalBooleanParameter cop theforeman/foreman#10066

Closed

This was referenced Jul 8, 2024

Fix some cops #11064

Closed

Fix some Minitest/ cops #11065

Merged

Fix Style/SoleNestedConditional cop #11066

Merged

Fix Style Cops #11067

Merged

archanaserver force-pushed the rubocop branch from 72693bb to 5055b0d Compare September 4, 2024 07:22

archanaserver force-pushed the rubocop branch from 5055b0d to 295f6a4 Compare September 11, 2024 09:35

ianballou reviewed Sep 18, 2024

View reviewed changes

pr-processor bot removed the Not yet reviewed label Sep 18, 2024

archanaserver force-pushed the rubocop branch from 295f6a4 to 460af38 Compare September 18, 2024 20:16

pr-processor bot added the Needs re-review label Sep 18, 2024

ehelms mentioned this pull request Oct 25, 2024

Fixes #37429 - Update theforeman-rubocop to v0.1.2 theforeman/foreman#9926

Open

14 tasks

archanaserver force-pushed the rubocop branch from 460af38 to c699227 Compare October 29, 2024 09:08

github-actions bot added the Packaging Change label Oct 29, 2024

archanaserver force-pushed the rubocop branch from c699227 to f231c3d Compare October 29, 2024 09:10

archanaserver force-pushed the rubocop branch from f231c3d to 8e22676 Compare November 5, 2024 06:54

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

archanaserver force-pushed the rubocop branch from 8e22676 to 909dfe3 Compare November 5, 2024 08:09

ianballou approved these changes Nov 6, 2024

View reviewed changes

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

ianballou merged commit 12b5a3d into Katello:master Nov 7, 2024
25 of 27 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

archanaserver commented May 29, 2024

archanaserver commented May 29, 2024

jeremylenz left a comment

chris1984 commented Jun 12, 2024

jeremylenz commented Jun 20, 2024

archanaserver commented Jun 25, 2024

jeremylenz commented Jun 25, 2024

archanaserver commented Jun 26, 2024

jeremylenz commented Jun 26, 2024

archanaserver commented Jul 8, 2024 •

edited

Loading

archanaserver commented Jul 8, 2024

jeremylenz commented Jul 9, 2024

ianballou commented Jul 9, 2024

archanaserver commented Jul 11, 2024

archanaserver commented Jul 18, 2024

archanaserver commented Sep 4, 2024

ianballou left a comment

ianballou commented Sep 18, 2024

ianballou commented Sep 18, 2024

ianballou commented Sep 18, 2024

archanaserver commented Sep 18, 2024

ehelms commented Sep 19, 2024 •

edited

Loading

ianballou commented Sep 19, 2024

archanaserver commented Sep 19, 2024

archanaserver commented Oct 29, 2024

jeremylenz commented Oct 29, 2024

archanaserver commented Nov 6, 2024

ianballou left a comment

archanaserver commented Nov 6, 2024

ianballou commented Nov 7, 2024

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

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

Conversation

archanaserver commented May 29, 2024

What are the changes introduced in this pull request?

Considerations taken when implementing this change?

What are the testing steps for this pull request?

archanaserver commented May 29, 2024

jeremylenz left a comment

Choose a reason for hiding this comment

chris1984 commented Jun 12, 2024

jeremylenz commented Jun 20, 2024

archanaserver commented Jun 25, 2024

jeremylenz commented Jun 25, 2024

archanaserver commented Jun 26, 2024

jeremylenz commented Jun 26, 2024

archanaserver commented Jul 8, 2024 • edited Loading

archanaserver commented Jul 8, 2024

jeremylenz commented Jul 9, 2024

ianballou commented Jul 9, 2024

archanaserver commented Jul 11, 2024

archanaserver commented Jul 18, 2024

archanaserver commented Sep 4, 2024

ianballou left a comment

Choose a reason for hiding this comment

ianballou commented Sep 18, 2024

ianballou commented Sep 18, 2024

ianballou commented Sep 18, 2024

archanaserver commented Sep 18, 2024

ehelms commented Sep 19, 2024 • edited Loading

ianballou commented Sep 19, 2024

archanaserver commented Sep 19, 2024

archanaserver commented Oct 29, 2024

jeremylenz commented Oct 29, 2024

archanaserver commented Nov 6, 2024

ianballou left a comment

Choose a reason for hiding this comment

archanaserver commented Nov 6, 2024

ianballou commented Nov 7, 2024

archanaserver commented Jul 8, 2024 •

edited

Loading

ehelms commented Sep 19, 2024 •

edited

Loading