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

Update HTTP gem requirement to allow version 5 #571

Merged
merged 3 commits into from Nov 24, 2022

Conversation

nielsslot
Copy link
Contributor

I wanted to use kubeclient in a project and noticed it required me to downgrade the version of the http gem to version 4. Version 5.0.0 of the http gem was released more than a year ago. Their changelog notes some breaking changes, but none of them seem to impact the usage in kubeclient.

This pull request also updates the rake development dependency to version 13. It looks like version 13 was released almost three years ago. Updating it was the easiest way to use version 5 of the http gem in kubeclient since it is dependent on the llhttp-ffi gem, which apparently is dependent on rake 13. I very much doubt that llhttp-ffi actually needs rake at runtime, but it is what it is. And actually updating rake here does make sense after three years.

@vindvaki
Copy link

vindvaki commented Oct 5, 2022

@cben Is there anything we can do to help get this merged?

@timflapper
Copy link

@cben Is there anything we can do to help get this merged?

I'd also be interested to know if there's anything that can be done to get this one released.

@cben
Copy link
Collaborator

cben commented Oct 28, 2022

Oh sorry, i totally dropped the ball on this. I have family plans today but hope to get it by Sunday.

@cben
Copy link
Collaborator

cben commented Oct 31, 2022

Resolved trivial (change in adjacent line) merge conflict.

Going over the breaking changes in http's changelog...

@cben
Copy link
Collaborator

cben commented Oct 31, 2022

  • ruby 2.5 tests timed out, sporadically times out (so far 2 times, passed 1 time) in RetryTest#test_can_watch_watches — maybe related as we use http for watches?

    • http 5.1.0 dropped ruby 2.5 support, but bundler correctly installed http 5.0.4 here.

    (I really should just drop EOL 2.5 from master branch, but I kinda still want it to work on v4.y branch)

@cben
Copy link
Collaborator

cben commented Oct 31, 2022

Compatibility

Well, there is one mild change, but it's to undocumented functionality ✔️

Kubeclient::Client instances have a .headers map that starts out initialized to {}. AFAICT we never change it, and there is no documentation that users may access/mutate it.

But if a user does put some extra headers there client["fOO_bAR"] = "baz", specifically if the key is a string, http 5.0 no longer normalizes the header name but will pass it to server exactly as spelled. (symbol keys are still normalized) httprb/http#576

(EDIT: this "functionality" only matters to non-RFC-compliant servers, probably irrelevant to k8s servers)

=> Since it's undocumented, I'm fine with this don't consider it a breaking change.
Also the long-term result is less "magic" happening, user will get what they asked for (that was the change's motivation in http) 👍

[Users: Don't take this as justification to mutate undocumented .headers. If you need it, open an issue to publicly expose the functionality.]

@cben
Copy link
Collaborator

cben commented Oct 31, 2022

(Oh I forgot to mention long-term we want to get off http gem entirely #488, blocked on question about WatchStream#finish)

@cben
Copy link
Collaborator

cben commented Oct 31, 2022

Hmm, the ruby 2.5 sporadic hanging in RetryTest#test_can_watch_watches does reproduce locally. I don't know yet if it's new with this PR, might well have existed before.

A recipe you can leave running until it hangs:

while timeout --verbose 3m env TESTOPTS="--verbose" bundle exec rake test; do 
  ruby -e 'puts "-"*200'
done

but that doesn't show traceback of where it was hanging.

@vindvaki
Copy link

vindvaki commented Nov 8, 2022

Hmm, the ruby 2.5 sporadic hanging in RetryTest#test_can_watch_watches does reproduce locally. I don't know yet if it's new with this PR, might well have existed before.

@cben Regardless of the cause, Ruby 2.5 (and even 2.6), are both EOL. If causing troubles in CI, maybe it's time to drop support?

@cben
Copy link
Collaborator

cben commented Nov 24, 2022

You're right => dropped 2.5 & 2.6 in #583. I also didn't notice that specific test is for Informer functionality doesn't exist on master branch, so my whole "but I maybe still care about 2.5 working there" argument was spurious.

It's just that my chronic deficit of time for kubeclient pushes me rely more on CI, and any silly thing like this is a distraction that ends up postponing progress :-(

Over these days I also saw various failures of this test on newer rubies, including without this PR. Opened #584 for that.

@cben cben closed this Nov 24, 2022
@cben cben reopened this Nov 24, 2022
@cben cben merged commit a83fd43 into ManageIQ:master Nov 24, 2022
@vindvaki
Copy link

Thanks for following up on this. Much appreciated 🙏

@dhstewart
Copy link
Contributor

@cben thank you for getting this merged in! 🎉

Do you plan to bump the version ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants