APPS-7784 Resolve dependency issues #430
Conversation
There was a problem hiding this comment.
Pull request overview
This PR upgrades several critical Ruby gem dependencies to resolve dependency compatibility issues. The changes involve major version upgrades for core web server components including Sinatra (v1-2 → v4), Thin (v1.8 → v2.0), and Rack (implicit → v3.0), along with an attempted upgrade of Nokogiri.
Key Changes:
- Major version upgrades to Sinatra 4.1.0, Thin 2.0.0, and explicit Rack 3.0.0 dependency
- Updated nokogiri version specification (though the specified version appears invalid)
- Corresponding Gemfile.lock updates reflecting new dependency tree including new transitive dependencies (base64, logger, rack-session)
Reviewed changes
Copilot reviewed 1 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| zendesk_apps_tools.gemspec | Updates runtime dependencies with major version bumps for thin, sinatra, rack, and nokogiri |
| Gemfile.lock | Reflects resolved dependency tree with upgraded versions and new transitive dependencies required by Sinatra 4.x and Rack 3.x |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
zendesk_apps_tools.gemspec
Outdated
| s.add_runtime_dependency 'thin', '~> 1.8.0' | ||
| s.add_runtime_dependency 'sinatra', '>= 1.4.6', '< 2.3.0' | ||
| s.add_runtime_dependency 'thin', '~> 2.0.0' | ||
| s.add_runtime_dependency 'sinatra', '~> 4.1.0' |
There was a problem hiding this comment.
The sinatra version is being upgraded from >= 1.4.6, < 2.3.0 to ~> 4.1.0, which is a major version jump (from v1-2 to v4). Sinatra 4.x introduced breaking changes including:
- Requires Ruby 3.0+
- Rack 3.x compatibility (now required)
- Changes to session handling and middleware stack
While the PR description mentions this is to resolve dependency issues, ensure that:
- The codebase has been tested with Sinatra 4.x
- Any breaking API changes have been addressed
- Session handling code has been reviewed for compatibility
The risk level should likely be higher than "low" given this is a multi-major-version upgrade.
There was a problem hiding this comment.
The changes has been tested locally.
zendesk_apps_tools.gemspec
Outdated
| s.add_runtime_dependency 'thin', '~> 2.0.0' | ||
| s.add_runtime_dependency 'sinatra', '~> 4.1.0' |
There was a problem hiding this comment.
Given the significant dependency upgrades (especially Sinatra from v1-2 to v4, and Thin from v1.8 to v2.0), test coverage is critical to ensure backward compatibility and catch breaking changes.
The PR description indicates tests have not been written yet. Please ensure:
- Existing test suites pass with the new dependency versions
- Additional tests are added to cover any changed behavior due to the upgrades
- Integration tests verify that the server (thin + sinatra) still functions correctly with Rack 3.x
The repository has comprehensive test coverage (spec/ and features/ directories), so maintaining this standard is important.
zendesk-subhasish
force-pushed
the
APPS-7760-resolve-dependency-issues
branch
from
2b38860 to
5499636
Compare
zendesk-subhasish
force-pushed
the
APPS-7760-resolve-dependency-issues
branch
from
29cc97b to
7ab5926
Compare
zendesk_apps_tools.gemspec
Outdated
| s.add_runtime_dependency 'faraday', '~> 0.17.5' | ||
| s.add_runtime_dependency 'execjs', '~> 2.7.0' | ||
| s.add_runtime_dependency 'nokogiri', '~> 1.13.10' | ||
| s.add_runtime_dependency 'nokogiri', '~> 1.18.8' |
There was a problem hiding this comment.
Consider using ~> 1.18 to allow minor version updates?
There was a problem hiding this comment.
We got a dependency alert on version < 1.18.8 , so it would good to have any minor version after 1.18.8
There was a problem hiding this comment.
Have you tried '~> 1.18', '>= 1.18.8'?
There was a problem hiding this comment.
I tried with ~> 1.18 with this, its takes the version 1.18.10, I think it will works in tis case then.
There was a problem hiding this comment.
The specification needs to be '~> 1.18', '>= 1.18.8' to not allow versions below 1.18.8. Otherwise the vulnerability can still exist in some installations.
There was a problem hiding this comment.
Thats right, I have updated according to that. Thanks Marcio!
zendesk-subhasish
changed the title
✌️
/cc @zendesk/vegemite
Description
This changes are to resolving dependencies issue by upgrading gems version.
References
Risks
[low] version upgrade for few gems