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

Added the ruby 3.3 pipeline #525

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

nikhil2611
Copy link
Contributor

Description

Added the ruby 3.3 pipeline

Related Issue

https://chefio.atlassian.net/browse/CHEF-12436

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Chore (non-breaking change that does not add functionality or fix an issue)

Checklist:

  • I have read the CONTRIBUTING document.
  • I have run the pre-merge tests locally and they pass.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • If Gemfile.lock has changed, I have used --conservative to do it and included the full output in the Description above.
  • All new and existing tests passed.
  • All commits have been signed-off for the Developer Certificate of Origin.

Signed-off-by: nikhil2611 <[email protected]>
@nikhil2611 nikhil2611 requested review from a team as code owners June 19, 2024 07:44

Quality Gate passed Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarQube

@@ -12,8 +12,8 @@ Gem::Specification.new do |s|
s.summary = %q{Plugin that adds functionality to Chef Infra's Knife CLI for configuring/interacting with nodes running Microsoft Windows}
s.description = s.summary

s.required_ruby_version = ">= 3.1"
s.add_dependency "chef", ">= 18.2"
s.required_ruby_version = ">= 3.3"

Choose a reason for hiding this comment

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

I wouldn't update this to 3.3 as it then wont be able to support chef-18. This does not control what version of ruby this works with other than setting a minimum version of ruby. Unless there is a breaking change or we are bumping for limiting versions we support this shouldn't get bumped up then. The ruby version installed on the system determines which ruby version this is working with. Expeditor is where you'd define the ruby version to run and test this on. @tpowell-progress can help explain this more if needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah @Stromweld we wont we merging these PRs now, these pr's will be merged after ruby 3.3 upgrade in chef-client and inspec.

Choose a reason for hiding this comment

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

in the end this isn't needed for that. >= 3.1 sets minimum ruby version for this gem, unless there is a reason to need to bump the minimum version such as a change that is only in a newer version it doesn't make sense to update the required_ruby_version as it'll create compatibility problems for chef 18 and older versions that are not running on ruby 3.3 but will still need to be supported.

Copy link
Contributor Author

@nikhil2611 nikhil2611 Jul 24, 2024

Choose a reason for hiding this comment

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

okay but ruby 3.1 will be going end of life by 2025-03-31, should we still keep that ?

Choose a reason for hiding this comment

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

I'd say yes since chef-client 18.x wont be EOL by then. Unless there are code changes that take advantage of newer ruby version changes that make it not backwards compatible with older ruby version then this value really doesn't need to change.

Copy link
Contributor

Choose a reason for hiding this comment

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

I can imagine that we will likely keep Ruby 3.1 in place for the foreseeable future, despite official EOL. I would in general be as conservative as possible with minimum Ruby requirements to avoid unnecessary headaches. Ruby 3.0 introduces some Ruby API changes that are sensible to require, but I would hesitate be too much higher than that. Considering this is updates going forward, Ruby 3.1 is understandable.

Copy link
Contributor

@tpowell-progress tpowell-progress left a comment

Choose a reason for hiding this comment

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

Ruby 3.1 and use 18-stable

@@ -8,7 +8,7 @@ if ENV["GEMFILE_MOD"]
puts "GEMFILE_MOD: #{ENV["GEMFILE_MOD"]}"
instance_eval(ENV["GEMFILE_MOD"])
else
gem "ohai", git: "https://github.com/chef/ohai", branch: "main"
gem "ohai", "~> 18.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be 18-stable

@@ -12,8 +12,8 @@ Gem::Specification.new do |s|
s.summary = %q{Plugin that adds functionality to Chef Infra's Knife CLI for configuring/interacting with nodes running Microsoft Windows}
s.description = s.summary

s.required_ruby_version = ">= 3.1"
s.add_dependency "chef", ">= 18.2"
s.required_ruby_version = ">= 3.3"
Copy link
Contributor

Choose a reason for hiding this comment

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

I can imagine that we will likely keep Ruby 3.1 in place for the foreseeable future, despite official EOL. I would in general be as conservative as possible with minimum Ruby requirements to avoid unnecessary headaches. Ruby 3.0 introduces some Ruby API changes that are sensible to require, but I would hesitate be too much higher than that. Considering this is updates going forward, Ruby 3.1 is understandable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants