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

bump http lib to remove circular dependencies #204

Merged
merged 1 commit into from
Apr 30, 2017

Conversation

grosser
Copy link
Contributor

@grosser grosser commented Oct 26, 2016

trying to avoid circular dependency in http library see httprb/http#353

@simon3z
Copy link
Collaborator

simon3z commented Oct 26, 2016

cc @moolitayer

@grosser
Copy link
Contributor Author

grosser commented Oct 27, 2016

same tests are red on master ... so good to go ?

@moolitayer
Copy link
Collaborator

Thanks @grosser.
Do you have a reproduction of the issue solved here?
I'm generally ok with this fix. @simon3z what do you think?
Anything in particular we need to test for bumping 'http' from 0.9.8 to 2.0.3?

It's the first time i'm seeing this integration failure and I couldn't figure out why it suddenly
happens(gem change?) @simon3z @grosser @jonmoter have you any idea why this happens?

@grosser
Copy link
Contributor Author

grosser commented Oct 27, 2016

reproduction is

ruby -w -r bundler/setup -r http -e 1

@grosser
Copy link
Contributor Author

grosser commented Oct 27, 2016

error is

assert(context.ssl_options[:cert_store].verify(context.ssl_options[:client_cert]))

tried adding timecop because maybe the cert is expired, but that did not work ...

@simon3z can you check the certs are indeed expired ... then update the certs and ideally document how they were created ?

@grosser
Copy link
Contributor Author

grosser commented Oct 28, 2016

alternatively can we set these tests to skip and move on instead of blocking all PRs on it ?

@simon3z
Copy link
Collaborator

simon3z commented Oct 28, 2016

@grosser we'll try to fix this asap.

@simon3z
Copy link
Collaborator

simon3z commented Nov 14, 2016

@grosser I think that you should update this PR according to the agreement in #206

@grosser
Copy link
Contributor Author

grosser commented Nov 14, 2016

agreement was to merge this when next kubeclient major release is done ?

@grosser
Copy link
Contributor Author

grosser commented Nov 14, 2016

alternatively we could say "> 0.9.8", "> 2.0.3" ... but that would be lying since we don't test on both versions

@simon3z
Copy link
Collaborator

simon3z commented Nov 15, 2016

agreement was to merge this when next kubeclient major release is done ?

@grosser I think it was mentioned on #206 to change this to ~> 2.0, but probably leaving it to ~> 2.0.3 is the same.

I think we could try to merge this ASAP. Is that ok with you @moleksyuk ?

@moleksyuk
Copy link

Hi @simon3z
Sorry for a long silence.

Unfortunately it won't work for me, because other third party dependencies required other newer version. It was 2 weeks ago, I upgraded manually to http library in logstash but got other issues.

Also, it's not required for me any more.

But thanks a lot guys!

@grosser
Copy link
Contributor Author

grosser commented Nov 15, 2016

updated to 2.0 ... which is the correct dependency in semver terms ...

On Tue, Nov 15, 2016 at 1:26 AM, Federico Simoncelli <
notifications@github.com> wrote:

agreement was to merge this when next kubeclient major release is done ?

@grosser https://github.com/grosser I think it was mentioned on #206
#206 to change this to ~> 2.0,
but probably leaving it to ~> 2.0.3 is the same.

I think we could try to merge this ASAP. Is that ok with you @moleksyuk
https://github.com/moleksyuk ?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#204 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAAsZ-gXvYHYU94Bb3tn8JykLnGZIM81ks5q-XrEgaJpZM4KhxO4
.

@simon3z
Copy link
Collaborator

simon3z commented Dec 23, 2016

@gabe-ochoa is this change OK for you?

@gabe-ochoa
Copy link

@simon3z Apologies for the super slow reply. This change works for my use case. We modified the usage of the tool chain that we're using kubeclient in such that it's isolated enough to not cause the dependency errors we were getting.

Great to see the dependency being upgraded.

@cben
Copy link
Collaborator

cben commented Apr 18, 2017

@msufa Is this OK or would it break your Celluloid + http.rb use case?

@msufa
Copy link
Contributor

msufa commented Apr 19, 2017

@cben the use-case was meant for https://github.com/zendesk/samson but has essentially been removed in zendesk/samson#1085

Since I'm no longer involved in that project I'll let @grosser have the final word, though I think this should work fine.

@grosser
Copy link
Contributor Author

grosser commented Apr 22, 2017

updated ... ready to merge ?

@cben
Copy link
Collaborator

cben commented Apr 23, 2017

Cool LGTM.

@grosser kubeclient README documents support for celluloid-io, do you think that needs any update?
Reading a bit more it seems http might still support Celluloid, but not officially due to not supporting timeouts [https://github.com/httprb/http/pull/268], but kubeclient uses http with nil (infinite) timeouts, so it might still work...
EDIT: just tested this PR, watch_pods seems to work with both default sockets and celluloid sockets.

@grosser
Copy link
Contributor Author

grosser commented Apr 23, 2017 via email

@moolitayer moolitayer mentioned this pull request Apr 25, 2017
1 task
@simon3z
Copy link
Collaborator

simon3z commented Apr 28, 2017

@cben @moolitayer can you review/approve this?

@simon3z simon3z merged commit b95eff5 into ManageIQ:master Apr 30, 2017
@grosser grosser deleted the grosser/http branch April 30, 2017 21:51
@cben
Copy link
Collaborator

cben commented Jan 21, 2018

We labeled v2.x/no as 0.9.8 is still bundle-installable and aside from runtime warnings passes kubeclient specs, so no need to force this bump on people in a minor release.

  • @moolitayer but we should relax dependency on 2.x to allow people to get newer version without the runtime warnings.

@moolitayer
Copy link
Collaborator

@cben so we will need a 2.x only PR, added to our list

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.

7 participants