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

[v2.x] backports for 2.6 release #289

Closed
wants to merge 24 commits into from
Closed

[v2.x] backports for 2.6 release #289

wants to merge 24 commits into from

Conversation

cben
Copy link
Collaborator

@cben cben commented Jan 22, 2018

Draft for upcoming 2.6.0 release (#281).
To be followed by merging #287.

Work notes for future

https://github.com/abonas/kubeclient/issues?utf8=%E2%9C%93&q=label%3Av2.x%2Fyes
Sorting manually by merge date.
($ hub issue --include-pulls -s '' -l v2.x/yes using hub is also handy but doesn't show merge date.)
Cherry-picking merge commits with:

git cherry-pick --mainline=1 $(git log --pretty='%H' origin/master --grep='Merge pull request #223')

EDIT: in future consider cherry-pick -x to link to original commit (though for PR merges the PR link is good enough)

Backported (newest last order)

#215 Code style: add parentheses around methods for test file v2.x/yes
#223 bump rubocop, fix offenses, use 2-space indent v2.x/yes
# Conflicts:
# .rubocop.yml
# lib/kubeclient/version.rb
rubocop fails a lot from this point, due to new cops & defaults.

#221 Include request info in exceptions to make it easy to see what reques… v2.x/yes
#229 Clarifying docs around process_template v2.x/yes
#231 relax requirement on recursive-open-struct v2.x/yes
#225 Remove limited entity list from Readme v2.x/yes
#232 Fix Minitest deprecation v2.x/yes
#236 bump rubocop to enforce what our style dictates v2.x/yes
#243 adjust readme to rubocop v2.x/yes
#205 make test output more readable by adding color v2.x/yes

#195 move exception into Kubeclient namespace v2.x/yes
- Test FAIL! see fix below
#233 Introduce Kubeclient::ResourceNotFoundError v2.x/yes
- Test FAIL! see fix below
#247 Support ruby 2.4 v2.x/yes
After above 3 PRs it's possible to completely revert backport changes of #271 (now closer to original #262) => tests PASS.

#261 Fixes warnings in test output (and a potential bug) v2.x/yes
#267 cascade delete entities with a delete_option parameter. v2.x/yes
#279 Recurse over arrays for watch notices v2.x/yes
#280 Catch EBADF when watch_stream fails v2.x/yes
#286 allow running tests with ruby test_foo.rb test v2.x/yes
#285 implement as: :raw for watch v2.x/yes
- Test FAIL on Ruby 2.0 — this commit used 2.1+ syntax.
=> Made small change e9817fd to use 2.0 syntax.

#288 Fix README comment on Kubeclient::HttpError bug v2.x/yes
#295 Add ruby 2.5.0 to Travis test matrix v2.x/yes

- Rubocop still errors.  Remaining errors were fixed on master by #253 and #269 but not backporting those.
=> Disabled couple cops on v2.x.

rake test passed locally after each backport except 3 FAIL mentioned above.

simon3z and others added 20 commits January 22, 2018 11:44
Code style: add parentheses around methods for test file
bump rubocop, fix offenses, use 2-space indent
# Conflicts:
#	.rubocop.yml
#	lib/kubeclient/version.rb
Include request info in exceptions to make it easy to see what reques…
Clarifying docs around process_template
…ent_on_recursive-open-struct

relax requirement on recursive-open-struct
Remove limited entity list from Readme
bump rubocop to enforce what our style dictates
make test output more readable by adding color
move exception into Kubeclient namespace
# Conflicts:
#	README.md
#	lib/kubeclient/common.rb
Introduce Kubeclient::ResourceNotFoundError
This reverts commit 263ff40 from ManageIQ#271.
Those backport changes are no longer needed:
- after backporting ManageIQ#195 and ManageIQ#233, v2.x now has
  Kubeclient::HttpError and Kubeclient::ResourceNotFoundError.
- after backporting ManageIQ#247, v2.x now tests with webmock 2.x,
  it takes `basic_auth` rather than user:pw in URL.
- ns/namespace change was just cosmetic.

In other words, v2.x branch is now closer to original ManageIQ#262.
Fixes warnings in test output (and a potential bug)
cascade delete entities with a delete_option parameter.
…_watch_notices

Recurse over arrays for watch notices
Catch EBADF when watch_stream fails
allow running tests with ruby test_foo.rb
@cben cben requested a review from moolitayer January 22, 2018 12:07
cben and others added 4 commits January 25, 2018 18:04
Fix README comment on Kubeclient::HttpError
Add ruby 2.5.0 to Travis test matrix
Make WatchStream.initialize as: keyword param optional,
ruby 2.0 didn't have mandatory keyword args.
It's internal API, and all .new() calls in kubeclient pass as:.
Backporting ManageIQ#223 bumped to rubocop with different defaults.
These rubocop errors are fixed on master by ManageIQ#253 and ManageIQ#269
but we didn't backport those.
@cben cben changed the title [WIP] cherry picks for 2.6 release cherry picks for 2.6 release Jan 25, 2018
@cben
Copy link
Collaborator Author

cben commented Jan 25, 2018

This is ready. Please review non-"Merge pull request #nnn" commits carefully!

Ended up disabling couple rubocops on v2.x.
An alternative could be backport only style changes from #253 (cben@536b281) + fix one place (cben@f5a05e5).
I feel weird about making extra style changes on release branch only to please rubocop.
(we did backport many style PRs but that was out of fear we'd otherwise get conflicts)

@moolitayer
Copy link
Collaborator

@cben please get more reviewers to review this. I'm on a course and flying next week so I have limited availability, regardless this is a large PR and should be reviewed by more then one person.

Why shouldn't we backport only minimal/needed changes and concentrate on releasing master?

@cben
Copy link
Collaborator Author

cben commented Jan 28, 2018

@grosser @jhernand @simon3z can you help reviewing?
Most are clean cherry picks, only the 223 one had conflicts.
There is one clean revert (see commit message).
And last two "[v2.x only]" extra commits by me need review.

EDIT: let me know if prefer smaller PRs and see a way to split this...

@cben cben changed the title cherry picks for 2.6 release [v2.x] cherry picks for 2.6 release Jan 28, 2018
@cben cben changed the title [v2.x] cherry picks for 2.6 release [v2.x] backports for 2.6 release Jan 28, 2018
@cben
Copy link
Collaborator Author

cben commented Jan 28, 2018

Why shouldn't we backport only minimal/needed changes and concentrate on releasing master?

Who'd be the target audience for "minimial needed changes" release?
We have 21 PRs here that are not released yet, that we deemed backward-compatible. About half of these are style & tests, but still, a lot of user-visible improvements.

Every user could have different opinion what are "minimial needed" changes (as an example, I think ManageIQ only "needs" 2 fixes plus cares about ruby 2 more for ruby 2.4 and 2.5 test coverage).
And then if someone asks for more, and we end doing another release with out of order backports, those are painful and add up to more work and risk :-(

If we release "maximal possible changes" — all 21 PRs — in 2.6, we "catch up to the debt".
This brings v2.x textually pretty close to master (+82 -39 lines).
After that, we can mostly forget about v2.x branch (and if we ever need another 2.x release it's not scary).

@moolitayer
Copy link
Collaborator

Who'd be the target audience for "minimial needed changes" release?

Any stable product out there using kubeclient.

We have 21 PRs here that are not released yet, that we deemed backward-compatible. About half of these are style & tests, but still, a lot of user-visible improvements.

That's why we are releasing master which we are going to do regardless.

As we are going to handle kubeclient release I would want to create a commitment to release master often enough, but would not like to commit to cheery picking that much. In the words of someone wise:
#281 (comment)

I accept we already discussed this and time was invested so will not object any longer.

Next time I do want to concentrate on releases from master.

@moolitayer
Copy link
Collaborator

New commits e9817fd & d39a9c1 Look good, the revert 2381575 as well.

Copy link
Collaborator

@simon3z simon3z left a comment

Choose a reason for hiding this comment

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

I haven't reviewed in detail but we discussed few of these PRs in person and the backport policy we put in place is good. 👍

@cben
Copy link
Collaborator Author

cben commented Feb 4, 2018

shelving this. did a 3.0 and doing a small 2.5.2 (see #281 (comment)).

If anyone wants additional 2.x release, let us know what you need.

@cben cben closed this Feb 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants