Skip to content

Conversation

@ccleung
Copy link
Contributor

@ccleung ccleung commented Jun 16, 2017

Need to upgrade dependencies for this gem, or else there will be a gem conflict with other public gems, e.g., latest google-cloud-storage requires faraday ~> 0.11 in it's chain of dependencies:

google-cloud-storage (~> 1.1) was resolved to 1.1.0, which depends on
      google-cloud-core (~> 1.0) was resolved to 1.0.0, which depends on
        google-cloud-env (~> 1.0) was resolved to 1.0.0, which depends on
          faraday (~> 0.11)

Issue: 622a19a Looks to have been resolved by ruby-oauth/oauth2#271 so we can remove the restriction on oauth2 dependency lock

s.add_runtime_dependency 'omniauth-oauth2', '~> 1.2'
s.add_runtime_dependency 'oauth2', '~> 1.1.0'
s.add_runtime_dependency 'omniauth-oauth2', '~> 1.4.0'
s.add_runtime_dependency 'oauth2', '~> 1.4.0'
Copy link
Member

Choose a reason for hiding this comment

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

Let's remove this dependency, we can let omniauth-oauth2 resolve it. It was added only because v1.2.0 of oauth2 introduced an issue on encoding which was later fixed by ruby-oauth/oauth2#271

@ccleung ccleung force-pushed the upgrade-dependencies branch from 8b82cdc to 8b3da0c Compare June 16, 2017 20:54

s.add_runtime_dependency 'omniauth-oauth2', '~> 1.2'
s.add_runtime_dependency 'oauth2', '~> 1.1.0'
s.add_runtime_dependency 'omniauth-oauth2', '~> 1.4.0'
Copy link
Member

Choose a reason for hiding this comment

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

omniauth/omniauth-oauth2@v1.2.0...v1.4.0

The change from 1.2.0 to 1.4.0 looks safe to me but we might need to loose ruby 2.1 support. Not sure how many app which are on 2.1 still uses that gem

Copy link
Contributor Author

@ccleung ccleung Jun 16, 2017

Choose a reason for hiding this comment

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

updated to lock this version to depend on 2.1.9 and greater, mirroring omniauth gem's requirements https://rubygems.org/gems/omniauth

@ccleung ccleung force-pushed the upgrade-dependencies branch from 8b3da0c to caf02c2 Compare June 16, 2017 20:55
@ccleung
Copy link
Contributor Author

ccleung commented Jun 16, 2017

i think travis ruby versions may need to be updated to test ruby >= 2.1.9

Copy link
Member

@Edouard-chin Edouard-chin left a comment

Choose a reason for hiding this comment

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

@ccleung ccleung force-pushed the upgrade-dependencies branch from be27f01 to 027e09a Compare June 16, 2017 21:10
@ccleung
Copy link
Contributor Author

ccleung commented Jun 16, 2017

updated, thanks for the feedback!

@ccleung ccleung requested review from Edouard-chin and JoePym June 19, 2017 15:41
Copy link
Contributor

@JoePym JoePym left a comment

Choose a reason for hiding this comment

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

What's the plan to bump the version of the gem? I feel given as we are deprecating support for a patch ruby version, we should bump the patch level of the gem.

@ccleung
Copy link
Contributor Author

ccleung commented Jun 19, 2017

@JoePym we should do a major bump i think, since it's a breaking backwards incompatible change with certain ruby versions?

@ccleung ccleung force-pushed the upgrade-dependencies branch from 027e09a to ecf4b5b Compare June 19, 2017 16:00
@shopify-admins shopify-admins requested a review from JoePym June 19, 2017 16:00
@ccleung
Copy link
Contributor Author

ccleung commented Jun 20, 2017

we will do a minor version bump for this as per discussion

@ccleung ccleung merged commit 6a87cae into master Jun 27, 2017
@ccleung ccleung deleted the upgrade-dependencies branch June 27, 2017 17:33
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.

3 participants