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

Fixes #36849 - Run GHA on Ruby 3.0 #9871

Closed

Conversation

ofedoren
Copy link
Member

@ofedoren ofedoren commented Oct 20, 2023

@ekohl, this is something I meant during the meeting. As a start point, we could use non-blocking GA to see the status of upgrade to Ruby 3.0. After we're happy with this, we could move the workflow to the shared workflows repo, or leave it here, since I don't think this will be re-used. The next step would be to add Ruby 3.0 to matrix.json and start using shared workflows for the plugins. I could try things out in foreman_webhooks then.

I've also included 2.7 here, since it seems something changed even between 2.7.4 we use in Jenkins and 2.7.8 GA uses.

One test will fail on Ruby 2.7. This may be due to the setup itself, but as a workaround we can use fog/fog-libvirt#133

Few tests will fail on Ruby 3.0. This is mostly because we need to improve safemode library.

Otherwise seems the transition should be smoother than we'd expect. Huge issues though because how kwargs are now treated. Some things must be written carefully, which we... didn't do much.

UPD: requires #9900.

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.

This roughly matches what my impression was. kwargs will be a big thing, not too many other issues.

.github/workflows/foreman.yml Outdated Show resolved Hide resolved
@ekohl ekohl changed the title Fixes #36849 - Set up Foreman GA with Ruby 3.0 Fixes #36849 - Set up CI with Ruby 3.0 Oct 23, 2023
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.

Some progress on this part: @evgeni and I are working on using .github/matrix.json. #9881 & #9880 do that.

So I'd like to split this PR in 2 parts: introduce GHA based CI that reads matrix.json, which can be blocking. Then the second part is to add Ruby 3.0 to matrix.json and makes sure it passes.

I'm also looking at reporting test failures, possibly to replace Jenkins altogether. https://github.com/marketplace/actions/publish-test-results looks promising.

@evgeni
Copy link
Member

evgeni commented Oct 31, 2023

Some progress on this part: @evgeni and I are working on using .github/matrix.json. #9881 & #9880 do that.

Those are now merged :)

@ofedoren
Copy link
Member Author

ofedoren commented Nov 2, 2023

@ekohl, is that what you meant? I mean, the first commit adds GHA using matrix.json. The second adds Ruby 3 to matrix and fixes.

I can create a separate PR with the first commit, so we can then deal with Ruby 3 here only.

@ofedoren ofedoren changed the title Fixes #36849 - Set up CI with Ruby 3.0 Fixes #36849 - Run GHA on Ruby 3.0 Nov 10, 2023
@ekohl
Copy link
Member

ekohl commented Nov 23, 2023

These are errors introduced in b566ea8:

 Error:
ApplicationHelperTest::documentation#test_0005_#documentation_url and new docs page:
ArgumentError: wrong number of arguments (given 2, expected 0..1)
    app/helpers/application_helper.rb:308:in `documentation_url'
    test/helpers/application_helper_test.rb:42:in `block (2 levels) in <class:ApplicationHelperTest>'

Error:
ApplicationHelperTest::documentation#test_0004_#documentation_url and new docs page:
ArgumentError: wrong number of arguments (given 2, expected 0..1)
    app/helpers/application_helper.rb:308:in `documentation_url'
    test/helpers/application_helper_test.rb:36:in `block (2 levels) in <class:ApplicationHelperTest>

The others are blocked on theforeman/safemode#44

@evgeni
Copy link
Member

evgeni commented Nov 23, 2023

Shall we merge the existing fixes from this pr (esp. models/host and db/migrate ones) in a separate PR? This would allow plugins to play around with Ruby 3 by adding it manually to the matrix include list while we resolve Safemode issues?

@ofedoren
Copy link
Member Author

ofedoren commented Nov 23, 2023

Do we really want to include such code changes before we branch? :)

That thing https://github.com/theforeman/foreman/pull/9871/files#diff-9daa2c365cb8a7cb177648fe53854dae6588080e089b01333768fcb5da8ff4aeR15-R19, it scares me.

@ekohl
Copy link
Member

ekohl commented Nov 24, 2023

Do we really want to include such code changes before we branch? :)

I think the db/* and test/* changes are safe

That thing https://github.com/theforeman/foreman/pull/9871/files#diff-9daa2c365cb8a7cb177648fe53854dae6588080e089b01333768fcb5da8ff4aeR15-R19, it scares me.

Same ;)

@ekohl
Copy link
Member

ekohl commented Nov 29, 2023

In #9923 I split some of the changes into separate PRs. Perhaps we can merge part of that?

@ofedoren ofedoren requested a review from a team as a code owner December 11, 2023 15:04
@ofedoren
Copy link
Member Author

Hm, seems to be green :)

@evgeni
Copy link
Member

evgeni commented Dec 11, 2023

Nice

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.

The method_missing part still scares me. I'll try to do some further digging.

@@ -12,7 +12,11 @@ def self.method_missing(method, *args, &block)
end
end
if type.constantize.respond_to?(method, true)
type.constantize.send(method, *args, &block)
if method.to_s =~ /\Afind_in_(.*)\Z/ && args.size == 1 && args[0].is_a?(Hash)
Copy link
Member

Choose a reason for hiding this comment

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

I see this is copied from elsewhere, but would this make sense?

Suggested change
if method.to_s =~ /\Afind_in_(.*)\Z/ && args.size == 1 && args[0].is_a?(Hash)
if method.to_s.start_with?('find_in_') && args.size == 1 && args[0].is_a?(Hash)

Gemfile Outdated Show resolved Hide resolved
@evgeni
Copy link
Member

evgeni commented Jan 3, 2024

rebased

@@ -33,13 +33,13 @@ def test_generate_link_for
end

test '#documentation_url and new docs page' do
url = documentation_url('TestSection', { type: 'plugin_manual', name: 'foreman_my_plugin', version: '1.2' })
url = documentation_url('TestSection', type: 'plugin_manual', name: 'foreman_my_plugin', version: '1.2')
Copy link
Member

Choose a reason for hiding this comment

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

I should have included these in 441f20a

@ekohl
Copy link
Member

ekohl commented Jan 18, 2024

Closing in favor of #9989

@ekohl ekohl closed this Jan 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants