Skip to content

Conversation

@rwaffen
Copy link
Member

@rwaffen rwaffen commented Oct 17, 2025

No description provided.

Co-authored-by: Tim Meusel <[email protected]>
@bastelfreak
Copy link
Member

@rwaffen can you please test this on a few modules with lots of ruby files? We need to ensure that there aren't any configured cops that cause warnings, or cops that cause lots of changes.

@rwaffen
Copy link
Member Author

rwaffen commented Oct 17, 2025

I'll try to find some...

@rwaffen
Copy link
Member Author

rwaffen commented Oct 17, 2025

I'll add some commits to test some cops out.

@rwaffen
Copy link
Member Author

rwaffen commented Oct 17, 2025

puuuhhh, was trying puppet-wildfly and was wondering why 98 files inspected, 1616 offenses detected, 1519 offenses autocorrectable ... but it seems this module does not care about rubocop this much even with the old version of voxpupuli-test XD

will search some other module

@rwaffen
Copy link
Member Author

rwaffen commented Oct 17, 2025

puppet-extlib looking okay (no warns, or errors), but newer rubocop finds newer things to nag about 🤔

@rwaffen
Copy link
Member Author

rwaffen commented Oct 17, 2025

puppet-icinga2 also looking okayish

@rwaffen
Copy link
Member Author

rwaffen commented Oct 17, 2025

puppet-yum looking okayish

@rwaffen
Copy link
Member Author

rwaffen commented Oct 17, 2025

puppet-systemd looking okayish

@rwaffen
Copy link
Member Author

rwaffen commented Oct 17, 2025

puppet-prometheus looking okayish

@rwaffen
Copy link
Member Author

rwaffen commented Oct 17, 2025

puppet-nginx looking okayish

@bastelfreak
Copy link
Member

Now the tricky question is: Do any of the new cops, that require manual intervention, provide any benefit? And can they be autofixed? Maybe it makes sense to disable the cops instead. Because if it takes us half a year to update the ruby files in all of our modules, we just won't do it. That also explains why the rubocop.yml is so long and why we rarely touched it in the past.

@rwaffen
Copy link
Member Author

rwaffen commented Oct 17, 2025

so you want to basically disable all cops after 1.50? and make 1.81 behave like 1.50? thats a bit strange to me.

@rwaffen
Copy link
Member Author

rwaffen commented Oct 20, 2025

TODOs after talking with @bastelfreak

  • try to run msync locally and exec rubocop -a within it
  • if we enroll it I will do the followup and maybe fixes if nessessary
  • rubocop.yml, not .rubocop.yml, need to be checked if all is still valid and good for us
  • we have round about 200 modules, try to do msync in chunks or with regext somehow to not do everything at once and fail

@rwaffen rwaffen self-assigned this Oct 20, 2025
Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It took me a while to figure out that RuboCop was provided as an API to modules. I'd still be tempted to separate the Ruby update and RuboCop into separate PRs for a cleaner changelog.

Another note on commit message: I'd say "Require Ruby 3.2+" instead of "update ruby".

RSpec/SpecFilePathFormat:
Enabled: false

RSpec/SpecFilePathSuffix:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we please keep this enabled? It avoids situations like puppetlabs/puppetlabs-postgresql#1633 and puppetlabs/puppetlabs-postgresql#1634. It was actually the RSpec/SpecFilePathFormat that was the "too picky" part and I'm glad they separated the cops into 2.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can do, just disabled it, as the "original" it was splitted from was also disabled.

@rwaffen
Copy link
Member Author

rwaffen commented Oct 20, 2025

can also split up, yes

@rwaffen
Copy link
Member Author

rwaffen commented Oct 21, 2025

@ekohl would you only move the gemspec require or also the targetrubyversion out of the pr? Or both?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants