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: specify dependency versions #35

Merged
merged 1 commit into from
Dec 9, 2019

Conversation

prikha
Copy link
Contributor

@prikha prikha commented Oct 11, 2019

BREAKING CHANGE: Requires graphql ~> 1.9.8 and google-protobuf ~> 3.7

There is no version specified for the dependencies. This is unpleasant for those trying to integrate the gem with existing codebase. Even though the library is its early development stage it would be nice to at least specify requirements. Versions could be adjusted later on, but it's way easier to spot a problem that to see misleading protobuf errors.

@prikha prikha force-pushed the tighten-dependency-version-constraints branch from a2edb3b to 9e1eac8 Compare October 21, 2019 11:54
@prikha prikha changed the title Specify dependency versions chore: specify dependency versions Oct 21, 2019
@prikha prikha force-pushed the tighten-dependency-version-constraints branch 2 times, most recently from 19f5b99 to 53e0d84 Compare October 22, 2019 10:43
@rylanc
Copy link
Contributor

rylanc commented Oct 22, 2019

@lennyburdette Any idea what the minimum version of google-protobuf is that would work with federated tracing?

Copy link
Contributor

@rylanc rylanc left a comment

Choose a reason for hiding this comment

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

This is technically going to be a breaking change, since it may require users to upgrade their versions of google-protobuf and graphql (and those upgrades may include breaking changes). I'd like to keep the required dependencies at the lowest reasonable versions. Personally, my company uses version 1.9.8 of grapqhl and things are working well. Since graphql-ruby sometimes introduces breaking changes in minor versions (and even patch versions), let's make it ~> 1.9.8. Unless someone else has a lower version they can report success with.

I'd like to nail down a good range for google-protobuf too. It seems like the gem uses traditional semantic versioning, so we may be OK with ~> 3.0. @lennyburdette does that sound right?

@lennyburdette
Copy link
Contributor

lennyburdette commented Oct 22, 2019

Using ~> 3.0 should work just fine, but I wouldn't say that google-protobuf uses strict semantic versioning. Glancing at the changelog, they've dropped support for older versions of Ruby and removed API in minor releases.

@prikha prikha force-pushed the tighten-dependency-version-constraints branch from 53e0d84 to fd956a6 Compare October 23, 2019 05:51
@prikha
Copy link
Contributor Author

prikha commented Oct 23, 2019

@lennyburdette @rylanc I get what you say, but the PR came from a need:

Version 3.6.0 clearly doesn't work

Failure/Error:
  add_file('apollo.proto', syntax: :proto3) do
    add_message 'mdg.engine.proto.Trace' do
      optional :start_time, :message, 4, 'google.protobuf.Timestamp'
      optional :end_time, :message, 3, 'google.protobuf.Timestamp'
      optional :duration_ns, :uint64, 11
      optional :root, :message, 14, 'mdg.engine.proto.Trace.Node'
      optional :signature, :string, 19
      optional :details, :message, 6, 'mdg.engine.proto.Trace.Details'
      optional :client_name, :string, 7
      optional :client_version, :string, 8

NoMethodError:
  undefined method `add_file' for #<Google::Protobuf::Internal::Builder:0x00007fbb4ca90f10>
  Did you mean?  add_enum

Version 3.7.0 and above seems to be good. So changed the minimum required version to 3.7.0

@prikha prikha force-pushed the tighten-dependency-version-constraints branch from fd956a6 to 6f40f9a Compare October 23, 2019 05:54
@prikha
Copy link
Contributor Author

prikha commented Oct 30, 2019

@lennyburdette @rylanc @noaelad ☝️

@prikha prikha requested a review from rylanc November 1, 2019 12:13
@prikha
Copy link
Contributor Author

prikha commented Nov 13, 2019

@lennyburdette @rylanc @noaelad ☝️

@rylanc rylanc changed the title chore: specify dependency versions fix: specify dependency versions Dec 5, 2019
Copy link
Contributor

@rylanc rylanc left a comment

Choose a reason for hiding this comment

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

Sorry for the delay. We'll need to set the version of google-protobuf to ~> 3.7 instead of ~> 3.7.0. If we leave the patch number, it means that 3.8 and above won't match. Also, it should mean that the Gemfile.lock version is unchanged.

@prikha prikha force-pushed the tighten-dependency-version-constraints branch from 6f40f9a to 4bcf196 Compare December 7, 2019 09:49
@prikha
Copy link
Contributor Author

prikha commented Dec 7, 2019

@rylanc done

@rylanc rylanc merged commit 0a29bb3 into Gusto:master Dec 9, 2019
rylanc pushed a commit that referenced this pull request Dec 9, 2019
# [1.0.0](v0.5.1...v1.0.0) (2019-12-09)

### Bug Fixes

* specify dependency versions ([#35](#35)) ([0a29bb3](0a29bb3))

### BREAKING CHANGES

* Requires graphql ~> 1.9.8 and google-protobuf ~> 3.7
@rylanc
Copy link
Contributor

rylanc commented Dec 9, 2019

🎉 This PR is included in version 1.0.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@rylanc rylanc added the released label Dec 9, 2019
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.

3 participants