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

Only allow patch releases for our dependencies #92

Merged
merged 2 commits into from
Dec 22, 2017
Merged

Conversation

schmidt-sebastian
Copy link
Contributor

@schmidt-sebastian schmidt-sebastian commented Dec 21, 2017

I hope this will help reduce future breakages as those with GRPC 1.8.0 (grpc/grpc-node#130).

@ghost ghost added the cla: yes This human has signed the Contributor License Agreement. label Dec 21, 2017
@stephenplusplus
Copy link
Contributor

@ofrobots what do you think about this approach?

@schmidt-sebastian can you link the relevant issue from grpc that explains the breakage?

@schmidt-sebastian
Copy link
Contributor Author

@schmidt-sebastian can you link the relevant issue from grpc that explains the breakage?

Sorry, I should have done that to begin with: grpc/grpc-node#130

We saw high errors numbers on Google Cloud Functions and many user reports after we automatically pulled in this update. There are some links to these reports in the GitHub issue.

@ofrobots
Copy link

If I understand correctly, grpc messed up by making a breaking behaviour change in a semver-minor update. This is terrible, but it is still a mistake made by gRPC. We should provide this feedback to the grpc folks (/cc @murgatroid99) and let them fix. Locking down all dependencies using ~ dependencies seems overkill. If gRPC continues break the semver contract, we should absolutely stop trusting their updates, and perhaps escalate.

The problem with a locking down dependencies is that in the npm module ecosystem fixes (bug fixes and security fixes) are typically provided on the tip of the major branch. If you are restricting to through2 as ~2.0.3 then you're not going to get any fixes after through2@2.1.0 happens. This will rapidly lead to the package being out of date and potentially insecure.

/cc @JustinBeckwith

@schmidt-sebastian
Copy link
Contributor Author

@ofrobots Sounds good. I will update this PR to allow future versions of GRPC patch releases for 1.7.x only (right now we lock to 1.7.1).

@codecov
Copy link

codecov bot commented Dec 22, 2017

Codecov Report

Merging #92 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##           master    #92   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          12     12           
  Lines        1599   1599           
=====================================
  Hits         1599   1599

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 76f8e0c...cbb0c2f. Read the comment docs.

@schmidt-sebastian schmidt-sebastian removed their assignment Dec 22, 2017
@schmidt-sebastian schmidt-sebastian merged commit 9bd8bad into master Dec 22, 2017
@ghost ghost removed the cla: yes This human has signed the Contributor License Agreement. label Dec 22, 2017
@stephenplusplus stephenplusplus deleted the semver branch December 26, 2017 18:42
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