Skip to content

Cleanup dependencies - #10171

Merged
jsvd merged 7 commits into
elastic:masterfrom
jsvd:cleanup_dependencies
Feb 5, 2019
Merged

Cleanup dependencies#10171
jsvd merged 7 commits into
elastic:masterfrom
jsvd:cleanup_dependencies

Conversation

@jsvd

@jsvd jsvd commented Nov 21, 2018

Copy link
Copy Markdown
Member

No description provided.

jsvd
jsvd commented Nov 21, 2018
View reviewed changes
Comment thread Gemfile.template Outdated
jsvd
jsvd commented Nov 21, 2018
View reviewed changes
Comment thread Gemfile.template Outdated
@jsvd
jsvd force-pushed the cleanup_dependencies branch from 17237ad to afe03fe Compare November 21, 2018 12:32
jsvd
jsvd commented Nov 21, 2018
View reviewed changes
Comment thread Gemfile.template Outdated
jsvd
jsvd commented Nov 21, 2018
View reviewed changes
Comment thread Gemfile.template Outdated
@jsvd
jsvd force-pushed the cleanup_dependencies branch from 7b7eeb2 to 5073328 Compare November 22, 2018 10:30
yaauie
yaauie reviewed Nov 27, 2018
View reviewed changes

@yaauie yaauie left a comment •
edited
Loading

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.

This PR changes quite a few of our dependencies to be more open-ended than I imagine you are intending; if we have other safeguards in place to make sure we don't accidentally consume major releases from these dependencies, my arguments may be moot.

Since the squiggle-arrow operator allows the most-precisely specified version field to be greater than or equal to what is specified, using it with a single version field (the major) presumably allows breaking changes when dependencies release new majors; this could produce hard-to-debug problems later on if/when new major versions are released.

EDIT: resolved.

Comment thread logstash-core/logstash-core.gemspec Outdated
Comment thread logstash-core/logstash-core.gemspec Outdated
Comment thread logstash-core/logstash-core.gemspec Outdated
@jsvd

jsvd commented Nov 27, 2018

Copy link
Copy Markdown
Member Author

Since the squiggle-arrow operator allows the most-precisely specified version field to be greater than or equal to what is specified, using it with a single version field (the major) presumably allows breaking changes when dependencies release new majors; this could produce hard-to-debug problems later on if/when new major versions are released.

The squiggle operator is a bit weird for majors, it behaves as "only allow this major". This can be demonstrated with:

jruby-9.2.3.0 :007 > Gem::Requirement.new("~> 1").satisfied_by?(Gem::Version.new("2.0.0"))
 => false 
jruby-9.2.3.0 :008 > Gem::Requirement.new("~> 1").satisfied_by?(Gem::Version.new("2"))
 => false 
jruby-9.2.3.0 :009 > Gem::Requirement.new("~> 1").satisfied_by?(Gem::Version.new("1.4"))
 => true 
jruby-9.2.3.0 :010 > Gem::Requirement.new("~> 1").satisfied_by?(Gem::Version.new("0.4"))
 => false 
jruby-9.2.3.0 :011 > Gem::Requirement.new("~> 1").satisfied_by?(Gem::Version.new("1.0"))
 => true

@jsvd
jsvd force-pushed the cleanup_dependencies branch from 5073328 to 5e557de Compare January 30, 2019 14:55
@jsvd

jsvd commented Jan 31, 2019

Copy link
Copy Markdown
Member Author

jenkins test this please

@jsvd jsvd changed the title [wip] Cleanup dependencies Cleanup dependencies Jan 31, 2019
@jsvd

jsvd commented Jan 31, 2019

Copy link
Copy Markdown
Member Author

@yaauie ci is green now, can you take another pass?

@jsvd
jsvd force-pushed the cleanup_dependencies branch from ce586e3 to 53e0977 Compare February 5, 2019 08:21
@jsvd

jsvd commented Feb 5, 2019

Copy link
Copy Markdown
Member Author

jenkins test this please

yaauie
yaauie approved these changes Feb 5, 2019
View reviewed changes

@yaauie yaauie left a comment

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.

LGTM 👍

Comment thread logstash-core/logstash-core.gemspec Outdated
Comment thread logstash-core/logstash-core.gemspec Outdated
@jsvd
jsvd merged commit 8d19e6c into elastic:master Feb 5, 2019
@jsvd
jsvd deleted the cleanup_dependencies branch February 5, 2019 16:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@yaauie yaauie yaauie approved these changes

Assignees

No one assigned

Labels

code cleanup

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@jsvd @yaauie