-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Bump gems to 6.0.0 #8558
Bump gems to 6.0.0 #8558
Conversation
I checked out your branch and still found some rc references
|
sorry didn't mean to close |
3e4ed00
to
008c7b1
Compare
The other versions are usually bumped in a separate PR relatively close to release. I'm not sure why we do things this way, maybe worth discussing tomorrow? |
In that case, LGTM. |
@jakelandis I did forget to bump the gem versions along with this. I sent in a new commit with those changes. Are you LGTM on those? |
Gemfile.jruby-2.3.lock.release
Outdated
@@ -529,21 +528,21 @@ GEM | |||
rack | |||
rack-test (0.7.0) | |||
rack (>= 1.0, < 3) | |||
rake (12.1.0) | |||
rake (12.2.1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm +1 on this. In fact, I'm thinking that I'm actually +1 on not bumping any gems between RC2 and 6.0.0. This would be a change to our release process, but I think it makes sense. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My thoughts...
I see RC->Release == patch release, at least the same rules should apply.
IMO patch release rules:
Plugins: New plugin patch version's and (case-by-case) minor plugin updates are good. They can always downgrade those if there is the bug. The exception here is if a patch/minor upgrade pinned a new transitive version.
For non-plugins gems, I would think to not bump them unless they are intentionally getting upgraded. For example in this change set there the Jackson version bump is intentional and should be in 6.0.0, as it fixes a regression #8439
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the tricky thing is there is no bundle update --only-logstash-plugins-and-other-things-that-matter
.
Thinking about it, I don't think this PR is the right place to have this discussion (even though I started it!), and we should debate this policy going forward, but not right now.
WDYT? @jordansissel can you chime in here?
Gemfile.jruby-2.3.lock.release
Outdated
@@ -49,7 +49,7 @@ GEM | |||
aws-sdk-v1 (1.67.0) | |||
json (~> 1.4) | |||
nokogiri (~> 1) | |||
backports (3.8.0) | |||
backports (3.10.3) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3.8.0 was from April of this year and is represents a multi-month jump to 3.10.3. I remember this exact question for different review.
@robbavey - Do you to remember discussions around backports version jump ? (or was it someone else?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jakelandis I do recall it - the same thing happened when we bumped to 5.6.4 (https://github.com/elastic/logstash/pull/8470/files). I remember we had a conversation about it - which I thought was here, but doesn't look like it.
The backports gem version history (https://rubygems.org/gems/backports/versions) shows it to have been completely unchanged from April up until October, when there was a flurry of versions committed over a period of 10 days.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple questions about rake and backport version bumps.
Can we perhaps just have a PR that bumps the version from rc2 to GA without touching dependencies? I don't see a reason why the artifact won't build. Also this would help us understand what new code (if any) is shipping after the ga rename. |
I would like to suggest that in general these version bumps should be straightforward and happen quickly (as they hold up other places in the stack). I think that mixing in dependency changes with them should not be done as it's clear it leads to debate that contributes to these version bumps not happening quickly. |
As for a PR that only bumps the version, I had opened #8579. |
To unblock the rest of the stack, I've merged d58206d, so the discussion on this PR can focus on the dependencies instead of the version bump. |
Thanks @jsvd for the merge, apologies for the holdup @jasontedor . I've edited our documented version bump policy to split this work into two parts going forward to prevent this issue in the future. |
008c7b1
to
6ee5789
Compare
logstash-core-plugin-api (>= 1.60, <= 2.99) | ||
logstash-filter-date (3.1.8) | ||
logstash-core-plugin-api (>= 1.60, <= 2.99) | ||
logstash-filter-de_dot (1.0.2) | ||
logstash-core-plugin-api (>= 1.60, <= 2.99) | ||
logstash-filter-dissect (1.0.12) | ||
logstash-filter-dissect (1.1.1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
jar-dependencies | ||
logstash-core-plugin-api (>= 2.1.1, <= 2.99) | ||
logstash-filter-dns (3.0.6) | ||
logstash-core-plugin-api (>= 1.60, <= 2.99) | ||
lru_redux (~> 1.1.0) | ||
logstash-filter-drop (3.0.4) | ||
logstash-core-plugin-api (>= 1.60, <= 2.99) | ||
logstash-filter-elasticsearch (3.1.6) | ||
logstash-filter-elasticsearch (3.2.0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -271,7 +271,7 @@ GEM | |||
logstash-input-dead_letter_queue (1.1.1) | |||
logstash-codec-plain | |||
logstash-core-plugin-api (>= 1.60, <= 2.99) | |||
logstash-input-elasticsearch (4.0.6) | |||
logstash-input-elasticsearch (4.1.0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm OK with the change to add metadata into here
https://github.com/logstash-plugins/logstash-input-elasticsearch/blob/master/CHANGELOG.md
@@ -322,7 +322,7 @@ GEM | |||
mail (~> 2.6.3) | |||
mime-types (= 2.6.2) | |||
stud (~> 0.0.22) | |||
logstash-input-jdbc (4.2.4) | |||
logstash-input-jdbc (4.3.0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This fixes behavior from here: logstash-plugins/logstash-input-jdbc#147
@jakelandis I've rebased this and pinned LMK if it is now LGTM |
6ee5789
to
8639398
Compare
Gemfile.lock
Outdated
@@ -0,0 +1,762 @@ | |||
PATH |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this intentional to commit the .lock file ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, that is a mistake
Gemfile.lock
Outdated
PATH | ||
remote: ./logstash-core | ||
specs: | ||
logstash-core (7.0.0.alpha1-java) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If intentional, probably don't want 7.x here (and below)
@@ -186,7 +186,7 @@ GEM | |||
rspec (~> 3.0) | |||
rspec-wait | |||
stud (>= 0.0.20) | |||
logstash-filter-aggregate (2.6.3) | |||
logstash-filter-aggregate (2.7.0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like a good and safe change: https://github.com/logstash-plugins/logstash-filter-aggregate/blob/master/CHANGELOG.md#270
@jakelandis thinking about it, since we now have build candidates for 6.0.0-GA this PR may just be too late. WDYT @jordansissel ? I'm thinking I retarget the pinning of |
Minus the rouge .lock file - LGTM |
8639398
to
f6e1633
Compare
LGTM |
Andrew Cholakian merged this into the following branches!
|
Version bump to 6.0.0 GA