Skip to content

Remove activesupport due to its volatility and potential to generate …#211

Merged
Nik08 merged 13 commits into
mainfrom
tp/remove-activesupport
Nov 3, 2025
Merged

Remove activesupport due to its volatility and potential to generate …#211
Nik08 merged 13 commits into
mainfrom
tp/remove-activesupport

Conversation

@tpowell-progress

@tpowell-progress tpowell-progress commented Oct 16, 2025

Copy link
Copy Markdown
Collaborator

…irreconcilable conflicts

Resolves #195 and indirectly resolves #198

Description

activesupport is volatile and chef-licensing makes minimal usage of it. When CVE fixes are issued for some of the web of projects but not others, we get the following:

Fetching gem metadata from https://rubygems.org/.
Resolving dependencies..............................................................................................................................................................
Bundler could not find compatible versions for gem "activesupport":
  In Gemfile:
    chef was resolved to 19.1.90, which depends on
      chef-licensing (>= 1.2.0) was resolved to 1.2.0, which depends on
        activesupport (~> 7.2, >= 7.2.2.1)
 
    oc-chef-pedant was resolved to 2.2.0, which depends on
      activesupport (>= 4.2.7.1, <= 7.1.3.2)
bundler: failed to load command: tasks/bin/run_external_test (tasks/bin/run_external_test)

The Gemfile.lock updated a decent amount, but those should be for development purposes only.

Related Issue

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.

Comment thread components/ruby/lib/chef-licensing/cache_adapter.rb Outdated
Comment thread components/ruby/lib/chef-licensing/file_cache.rb Outdated
Comment thread components/ruby/lib/chef-licensing/file_cache.rb Outdated
Comment thread components/ruby/lib/chef-licensing/string_refinements.rb Outdated
Comment thread components/ruby/lib/chef-licensing/string_refinements.rb Outdated

@jaymzh jaymzh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🎉

@jaymzh

jaymzh commented Oct 17, 2025

Copy link
Copy Markdown
Contributor

You have a minor lint failure, but other than that, this looks ready to go.

@rahulgoel1

Copy link
Copy Markdown

@Nik08 @Vasu1105 can you pls review/ approve pls ? This is going to block chef-19 upgrade for inspec 7

@Nik08

Nik08 commented Oct 24, 2025

Copy link
Copy Markdown
Collaborator

@tpowell-progress @johnmccrae The changes look good. I have 2 concerns here:

  • Can we separate the lint changes from this PR since they are already covered here Updating linting and cookstyle support #212? This will make the code review much easier and specific to the intended change, which is the activesupport replacement
  • I see moneta is being used as a compatible storage with Faraday::HttpCache. Will it be possible to cover some test cases which also show the proper functioning of this with Faraday::HttpCache? Or have you done a round of testing with the airgap environment and these changes in caching? We do use headers, which are useful for expiration via Faraday::HttpCache, and just wanted to make sure that functionality is not affected by this integration with Moneta.

@sathish-progress sathish-progress left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This PR has too many changes that is out of scope for removal of activesupport dependency that is most probably coming from #212. Either this PR should be rebased against main or against jfm/chef19-cookstyle.

Comment thread .github/workflows/ruby-tests.yml Outdated
@@ -0,0 +1,123 @@
name: Ruby Tests

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please move the tests from here to spec files. We have existing verify pipelines which will tc of running these specs

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I see the tests are added in the spec. We don't need this.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Weird. Something must not have been firing because I wasn't getting any results in GA initially.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

removed the GitHub workflow in e98a83f


module ChefLicensing
class ListLicenseKeys
using StringRefinements

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nitpick, I see we use string refinements only here, and only for the purpose of pluralize may be we should just call it as a util and use the util directly something like below,
Chef::Licensing::StringUtils.present?(str)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

The reason I did it as a string refinement was because was one of the prior uses for ActiveSupport. I'm absolutely fine with a module with an explicit method call, but assumed that that wasn't the desire of the original code.

Comment thread components/ruby/chef-licensing.gemspec Outdated
spec.add_dependency "faraday", ">= 1", "< 2"
spec.add_dependency "faraday-http-cache"
spec.add_dependency "activesupport", "~> 7.2", ">= 7.2.2.1"
spec.add_dependency "moneta", "~> 1.6"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I understand we are removing activesupport to avoid external dependency issues. But at the same time we are introducing a new dependency that has not received any updates in last 2 years. How can we ensure this receives latest security patches and updates ?

@@ -0,0 +1,61 @@
Test harness notes for the chef-licensing specs

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@sathish-progress

Copy link
Copy Markdown
Contributor

Also as discussed with @Nik08 offline, adding Bump Version Minor label to this.

@rishichawda rishichawda force-pushed the tp/remove-activesupport branch from 2606da1 to dfd8e31 Compare October 29, 2025 04:59
@rishichawda

Copy link
Copy Markdown
Member

I've replaced the moneta implementation with pstore. details are in the commit message for dfd8e31 (had to force push my commit because I missed putting the pstore pin info in my commit message). pinning pstore to ~> 0.1.1 because we support ruby 3.1 and that's the pstore version ruby 3.1.x shipped.

cc: @tpowell-progress

tpowell-progress and others added 13 commits November 3, 2025 11:30
…irreconcilable conflicts

Signed-off-by: Thomas Powell <thomas.powell@progress.com>
Signed-off-by: Thomas Powell <thomas.powell@progress.com>
Signed-off-by: Thomas Powell <thomas.powell@progress.com>
Signed-off-by: Thomas Powell <thomas.powell@progress.com>
Signed-off-by: Thomas Powell <thomas.powell@progress.com>
Signed-off-by: Thomas Powell <thomas.powell@progress.com>
Signed-off-by: Thomas Powell <thomas.powell@progress.com>
Signed-off-by: Thomas Powell <thomas.powell@progress.com>
Signed-off-by: John McCrae <john.mccrae@progress.com>
moneta seems to be an overkill for our use case. we can do well with pstore instead for storing the key value pairs into file while staying compatible with faraday's http cache interface. this change also adds the pstore gem to gemspec and pins it to ~>0.1.1 because that's the version that was shipped with ruby 3.1.*.

Signed-off-by: Rishi Kumar Chawda <rishichawda@users.noreply.github.com>
Signed-off-by: Rishi Kumar Chawda <rishichawda@users.noreply.github.com>
we already have those in buildkite. discussion: https://github.com/chef/chef-licensing/pull/211/files#r2459618792

Signed-off-by: Rishi Kumar Chawda <rishichawda@users.noreply.github.com>
chef-licensing was failing with a `'lookup_middleware': :json is not registered on Faraday::Request` error that was coming from the get connection method in rest client. faraday_middleware gems seems to be the one responsible to provide this json middleware https://github.com/lostisland/faraday_middleware/wiki/Changes-0.8. pinning faraday_middleware to ~>1.0.

Signed-off-by: Rishi Kumar Chawda <rishichawda@users.noreply.github.com>
@rishichawda rishichawda force-pushed the tp/remove-activesupport branch from 756b506 to a29c0b9 Compare November 3, 2025 06:06
@sathish-progress sathish-progress requested review from Copilot and removed request for Nik08 November 3, 2025 06:12

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull Request Overview

This PR removes the activesupport dependency from the chef-licensing gem to resolve dependency conflicts that occur when different projects require incompatible versions of ActiveSupport. The changes implement minimal custom replacements for the specific ActiveSupport features that were being used.

Key changes:

  • Replace ActiveSupport cache with a custom PStore-based adapter
  • Implement basic string pluralization using refinements instead of ActiveSupport::Inflector
  • Update dependencies in gemspec to remove activesupport and add faraday_middleware and pstore

Reviewed Changes

Copilot reviewed 8 out of 9 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
chef-licensing.gemspec Removes activesupport dependency and adds faraday_middleware and pstore dependencies
lib/chef-licensing/pstore_adapter.rb Implements custom cache adapter using PStore to replace ActiveSupport::Cache
lib/chef-licensing/string_refinements.rb Provides basic string pluralization to replace ActiveSupport::Inflector
lib/chef-licensing/restful_client/base.rb Updates to use the new PStore adapter instead of ActiveSupport::Cache
lib/chef-licensing/list_license_keys.rb Adds usage of the new string refinements module
spec/chef-licensing/activesupport_replacement_spec.rb Adds comprehensive tests for the new PStore adapter and string refinements
spec/README.md Documents test setup and best practices for the spec suite
.rubocop.yml Adds introductory comment

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +52 to +69
end

RSpec.describe ChefLicensing::PStoreAdapter do
let(:cache_dir) { Dir.mktmpdir }
let(:adapter) { described_class.new(cache_dir) }

after do
FileUtils.rm_rf(cache_dir)
end

it "provides the same interface as ActiveSupport::Cache" do
adapter.write("test_key", "test_value")
expect(adapter.read("test_key")).to eq("test_value")
expect(adapter.exist?("test_key")).to be true

adapter.delete("test_key")
expect(adapter.read("test_key")).to be_nil
end

Copilot AI Nov 3, 2025

Copy link

Choose a reason for hiding this comment

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

This second RSpec.describe block duplicates the PStoreAdapter test setup and should be merged with the first block (lines 5-52) to avoid code duplication. The interface compatibility test can be added as another describe block within the existing structure.

Suggested change
end
RSpec.describe ChefLicensing::PStoreAdapter do
let(:cache_dir) { Dir.mktmpdir }
let(:adapter) { described_class.new(cache_dir) }
after do
FileUtils.rm_rf(cache_dir)
end
it "provides the same interface as ActiveSupport::Cache" do
adapter.write("test_key", "test_value")
expect(adapter.read("test_key")).to eq("test_value")
expect(adapter.exist?("test_key")).to be true
adapter.delete("test_key")
expect(adapter.read("test_key")).to be_nil
end
describe "interface compatibility with ActiveSupport::Cache" do
it "provides the same interface as ActiveSupport::Cache" do
adapter.write("test_key", "test_value")
expect(adapter.read("test_key")).to eq("test_value")
expect(adapter.exist?("test_key")).to be true
adapter.delete("test_key")
expect(adapter.read("test_key")).to be_nil
end
end

Copilot uses AI. Check for mistakes.
Comment on lines +9 to +20
# Simple pluralization rules
case downcase
when /s$/, /sh$/, /ch$/, /x$/, /z$/
"#{self}es"
when /[^aeiou]y$/
"#{self[0..-2]}ies"
when /f$/
"#{self[0..-2]}ves"
when /fe$/
"#{self[0..-3]}ves"
else
"#{self}s"

Copilot AI Nov 3, 2025

Copy link

Choose a reason for hiding this comment

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

The downcase method is called on the string but the original case is used in the return values. This will cause incorrect capitalization in pluralized strings. For example, 'Day'.pluralize(2) will return 'Days' but should preserve the original case as 'Days'.

Suggested change
# Simple pluralization rules
case downcase
when /s$/, /sh$/, /ch$/, /x$/, /z$/
"#{self}es"
when /[^aeiou]y$/
"#{self[0..-2]}ies"
when /f$/
"#{self[0..-2]}ves"
when /fe$/
"#{self[0..-3]}ves"
else
"#{self}s"
# Determine the pluralized form in lowercase
word = self
plural =
case word.downcase
when /s$/, /sh$/, /ch$/, /x$/, /z$/
"#{word}es"
when /[^aeiou]y$/
"#{word[0..-2]}ies"
when /f$/
"#{word[0..-2]}ves"
when /fe$/
"#{word[0..-3]}ves"
else
"#{word}s"
end
# Preserve original capitalization pattern
if word.match?(/\A[A-Z]+\z/)
plural.upcase
elsif word.match?(/\A[A-Z][a-z]+\z/)
plural.capitalize
elsif word.match?(/\A[a-z]+\z/)
plural.downcase
else
plural

Copilot uses AI. Check for mistakes.
@Nik08 Nik08 merged commit 8236024 into main Nov 3, 2025
6 checks passed
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.

activesupport 7.2 is part of a Rails version that is EOL in November Remove activesupport from chef-licensing

8 participants