Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Ruby 3.0 + Puppet 7 & 3.2 + Puppet 8 to CI #369

Merged
merged 1 commit into from
Jan 3, 2024

Conversation

archanaserver
Copy link
Contributor

Update Ruby ~> 3 and Puppet ~> 8

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.

We still want to keep testing on the old versions, just add the new versions.

@archanaserver archanaserver changed the title Update matrix versions [WIP] Update matrix versions Nov 21, 2023
@archanaserver archanaserver force-pushed the ruby3-puppet8-upgrade branch 2 times, most recently from 39721f9 to 65edc6c Compare November 21, 2023 08:35
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.

Instead of [WIP] you can also mark the PR as a draft (https://github.blog/2019-02-14-introducing-draft-pull-requests/).

.github/workflows/ci.yaml Outdated Show resolved Hide resolved
@archanaserver archanaserver changed the title [WIP] Update matrix versions Update matrix versions Nov 24, 2023
@evgeni
Copy link
Member

evgeni commented Nov 25, 2023

The tests execute now and the failure is related.

The internal kafo configure Module is not marked as compatible with Puppet 8:

"version_requirement": ">= 4.5.0 < 8.0.0"

Not sure if just bumping this would be sufficient, but certainly worth a try

@archanaserver archanaserver force-pushed the ruby3-puppet8-upgrade branch 3 times, most recently from cd67282 to 9e99a54 Compare December 19, 2023 07:13
@archanaserver
Copy link
Contributor Author

@ekohl @evgeni is there a way to check, if the test was still skipped on the previous changes?

@evgeni
Copy link
Member

evgeni commented Dec 19, 2023

@ekohl @evgeni is there a way to check, if the test was still skipped on the previous changes?

What do you mean by "on the previous changes"? The ones before you pushed today?

You can see previous runs in https://github.com/theforeman/kafo/actions

And they did not skip the "Tests" part, as there "Rubocop" succeeded.

Seems Rubocop 1.59 is rather new (https://github.com/rubocop/rubocop/releases/tag/v1.59.0) and triggers those errors.
I know they passed with 1.56.4, but no idea if any version inbetween is also good.

We should archive the Gemfile.lock generated in theforeman/actions/.github/workflows/rubocop.yml, so we can trace such issues easier.

@evgeni
Copy link
Member

evgeni commented Dec 19, 2023

theforeman/actions#25

@archanaserver
Copy link
Contributor Author

I can't see this rubocop failure in my local, also i just remembered that i've already fix these two cops here https://github.com/theforeman/kafo/blob/master/.rubocop.yml#L156-L160, don't know why it is failing here?

@ehelms
Copy link
Member

ehelms commented Jan 2, 2024

This is what fixed the cops erroring for me:

diff --git a/lib/kafo/kafo_configure.rb b/lib/kafo/kafo_configure.rb
index be0a469..9cbdb70 100644
--- a/lib/kafo/kafo_configure.rb
+++ b/lib/kafo/kafo_configure.rb
@@ -120,7 +120,7 @@ module Kafo
       request_config_reload if applied_total > 0
 
       if ARGV.include?('--migrations-only')
-        verbose = (ARGV.include?('--verbose') || ARGV.include?('-v'))
+        verbose = ARGV.include?('--verbose') || ARGV.include?('-v')
         Logging.setup(verbose: verbose)
         self.class.logger.notice('Log buffers flushed')
         self.class.exit(0)
diff --git a/lib/kafo/param.rb b/lib/kafo/param.rb
index 363e16b..cf5047f 100644
--- a/lib/kafo/param.rb
+++ b/lib/kafo/param.rb
@@ -161,7 +161,7 @@ module Kafo
 
     def normalize_value(value)
       case value
-        when ::HighLine::String  # don't persist highline extensions
+        when ::HighLine::String # don't persist highline extensions
           value.to_s
         when Array
           value.map { |v| normalize_value(v) }
diff --git a/lib/kafo/progress_bar.rb b/lib/kafo/progress_bar.rb
index a10810e..ec32f71 100644
--- a/lib/kafo/progress_bar.rb
+++ b/lib/kafo/progress_bar.rb
@@ -34,7 +34,7 @@ module Kafo
       @all_lines += 1
 
       # we print every 20th line during installation preparing otherwise only update at EVALTRACE_START
-      update_bar = (@total == :unknown && @all_lines % 20 == 0)
+      update_bar = @total == :unknown && @all_lines % 20 == 0
       force_update = false
 
       if (line_monitor = MONITOR_RESOURCE.match(line))

@archanaserver
Copy link
Contributor Author

@ehelms thanks, i got it!

Copy link
Member

@evgeni evgeni left a comment

Choose a reason for hiding this comment

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

One could argue this should be 3 commits ("make code compliant with latest rubocop", "mark kafo_configure module as compatible with puppet 8" and "include Ruby 3 and Puppet 8 in test matrix"), but I am not going to nitpick here :)

@ehelms ehelms merged commit 3b907d4 into theforeman:master Jan 3, 2024
6 checks passed
@archanaserver archanaserver deleted the ruby3-puppet8-upgrade branch January 3, 2024 20:00
@archanaserver
Copy link
Contributor Author

Thank you for the review.

@ekohl ekohl changed the title Update matrix versions Add Ruby 3.0 + Puppet 7 & 3.2 + Puppet 8 to CI Jan 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants