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

fix: update http gem dependency to 4.4.1 #33

Merged
merged 1 commit into from
Mar 15, 2021

Conversation

MosesMendoza
Copy link
Contributor

Hi there,

I was integrating with the Ruby SDK in a rails app, which runs ruby 2.7.2. I wasn't able to initialize an IamAuthenticator object in that lib and traced it down to the http gem. Relevant stack trace portion:

[5] pry(#<Api::V1::AnalysesController>)> exception.backtrace
=> ["/Users/moses/development/foo/vendor/bundle/ruby/2.7.0/gems/http-4.1.1/lib/http/response/body.rb:52:in `force_encoding'",
 "/Users/moses/development/foo/vendor/bundle/ruby/2.7.0/gems/http-4.1.1/lib/http/response/body.rb:52:in `to_s'",
 "/Users/moses/development/foo/vendor/bundle/ruby/2.7.0/gems/ibm_cloud_sdk_core-1.1.2/lib/ibm_cloud_sdk_core/token_managers/jwt_token_manager.rb:85:in `request'",
 "/Users/moses/development/foo/vendor/bundle/ruby/2.7.0/gems/ibm_cloud_sdk_core-1.1.2/lib/ibm_cloud_sdk_core/token_managers/iam_token_manager.rb:54:in `request_token'",
 "/Users/moses/development/foo/vendor/bundle/ruby/2.7.0/gems/ibm_cloud_sdk_core-1.1.2/lib/ibm_cloud_sdk_core/token_managers/jwt_token_manager.rb:30:in `token'",
 "/Users/moses/development/foo/vendor/bundle/ruby/2.7.0/gems/ibm_cloud_sdk_core-1.1.2/lib/ibm_cloud_sdk_core/token_managers/iam_token_manager.rb:33:in `initialize'",
 "/Users/moses/development/foo/vendor/bundle/ruby/2.7.0/gems/ibm_cloud_sdk_core-1.1.2/lib/ibm_cloud_sdk_core/authenticators/iam_authenticator.rb:28:in `new'",
 "/Users/moses/development/foo/vendor/bundle/ruby/2.7.0/gems/ibm_cloud_sdk_core-1.1.2/lib/ibm_cloud_sdk_core/authenticators/iam_authenticator.rb:28:in `initialize'",

I came across this fix that references the frozen string error. It was backported to http gem version 4.3.0. I updated the dependency in Ruby SDK but there too but it conflicted with the dependency in this project so I updated this one as well. After this fix (and the one in that repo, PR incoming) the code ran fine in rails on ruby 2.7.2 I'm running the project from gem sources in github at the moment, but here's a PR in case you want to update officially. This PR changes the dep to 4.4.x, but 4.1 -> 4.4 should be backwards-compatible. I'm not an expert at minitest but it seems all the tests pass with these changes (at least the ones triggered by bundle exec rake.

Cheers

Signed-off-by: Moses Mendoza <mendoza.moses@gmail.com>
@padamstx
Copy link
Member

padamstx commented Mar 5, 2021

@repjarms @Mikemosca Could you guys please review/merge as appropriate? Thanks!

@Mikemosca
Copy link

A couple of points.

Do we know for sure this http dependency upgrade in core is backward compatible? Will it break the existing ruby sdk 2.5/2.6 code base? Does the ruby sdk need an upgrade also to be compatible with core (I see a similar PR was put it for ruby sdk)? If so does this warrant a major release to sync up core and sdk?

I guess we can just wing it should work. Maybe we need to do some testing on the new core before we create the release. It's up to you guys. I don't see the rush here as we state in the readme we only support 2.5 and 2.6 and ruby 2.7 has been not qualified. Perhaps more discussion and planning is needed for next week?

@padamstx
Copy link
Member

padamstx commented Mar 5, 2021

Hi @Mikemosca, it looks like the ruby core automated builds are performed on 3 different versions of ruby (2.3.7, 2.4.5, and 2.5.3).
Perhaps we need to add one for ruby 2.6 and (eventually) 2.7?
The SDK squad would look to your team to validate that this PR doesn't break anything since it's really just the watson sdk that uses the core i think.

@Mikemosca
Copy link

ok, will talk to the bosses and create issue to verify the the new core. Perhaps we can fit it in next week

@Mikemosca
Copy link

I created a local core gem with the http upgrade and ran the ruby sdk tests and they passed. So it seems to be backward compatible. So will approve the change.

  1. The next step is for you guys to generate a new core release. - 1.1.3
  2. Once that is done, I can pick up the new release and
    a. make a similar http upgrade to the rudy sdk
    b. Verify there are no regressions for existing ruby versions
    c. Introduce 2.7 ruby as supported to the SDK and verify all tests pass
    d. update README with version info for 2.7 and create a new SDK release

Copy link

@Mikemosca Mikemosca left a comment

Choose a reason for hiding this comment

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

See new comment for next steps

@jeff-arn jeff-arn merged commit 3b94f63 into IBM:main Mar 15, 2021
@watson-github-bot
Copy link
Collaborator

🎉 This PR is included in version 1.1.3 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

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