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

Failing metadata plugin results in UNAVAILABLE #88

Closed

Conversation

jtattermusch
Copy link
Contributor

@jtattermusch jtattermusch commented Nov 13, 2017

Context: grpc/grpc#13322

Needs to be merged before grpc/grpc#13363. The change is a little awkward, but since grpc-node now lives in a different repository, we don't have merge atomicity anymore and we can't really come up anything better without breaking the test temporarily.

@jtattermusch
Copy link
Contributor Author

CC @jboeuf

@murgatroid99
Copy link
Member

We're just going to have to accept that that test is going to break sometimes. I don't think we need to make this kind of transitional change. We should just make the actual change with the next submodule update.

@jtattermusch
Copy link
Contributor Author

I'm fine with breaking the tests for duration of ~minutes (basically having all the PRs ready to merge), but knowingly introducing a breakage for longer time is wrong. What is the right ordering of PRs to minimize the impact?
Is this correct:

  1. merge this PR (after modifying it to only accept UNAVAILABLE status) - that will break the grpc/grpc tests temporarily
  2. merge Revert "Revert "Switching from UNAUTHENTICATED to UNAVAILABLE for auth metadata failure"" grpc#13363 shortly after 1) which will fix the tests in both grpc/grpc and grpc-node.

@murgatroid99
Copy link
Member

I have opened #90, which is my preferred solution. You should merge your PR on grpc/grpc, then I will update that one and merge it. The grpc/grpc tests will be broken temporarily.

@nicolasnoble
Copy link
Member

I would side with @murgatroid99 here that we shouldn't try making commits that handles two things at once. Unless the change is too big, preparing the change such as #90 and committing grpc/grpc#13363 in almost lockstep is the better way to handle this.

@jtattermusch
Copy link
Contributor Author

Closing in favor of #90.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants