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

Use lower-case gRPC header keys #2212

Merged
merged 2 commits into from
Aug 27, 2016
Merged

Use lower-case gRPC header keys #2212

merged 2 commits into from
Aug 27, 2016

Conversation

yang-g
Copy link
Contributor

@yang-g yang-g commented Aug 26, 2016

As upper-case letters are illegal per http2 spec.

As upper-case letters are illegal per http2 spec.
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Aug 26, 2016
@yang-g
Copy link
Contributor Author

yang-g commented Aug 27, 2016

#2204

@tseaver tseaver added the grpc label Aug 27, 2016
@tseaver
Copy link
Contributor

tseaver commented Aug 27, 2016

@yang-g Thanks for the fix! Travis is going to barf whenever it gets around to running, because the unit tests assert the upper-case field names:

======================================================================
FAIL: test___call__ (gcloud.test__helpers.TestMetadataPlugin)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/tseaver/projects/agendaless/Google/src/gcloud-python/gcloud/test__helpers.py", line 845, in test___call__
    self.assertEqual(callback_args, [(cb_headers, None)])
AssertionError: Lists differ: [([('authorization', 'Bearer F... != [([('Authorization', 'Bearer F...

First differing element 0:
([('authorization', 'Bearer FOOBARBAZ'), ('user-agent', 'USER_AGENT')], None)
([('Authorization', 'Bearer FOOBARBAZ'), ('User-agent', 'USER_AGENT')], None)

- [([('authorization', 'Bearer FOOBARBAZ'), ('user-agent', 'USER_AGENT')], None)]
?      ^                                      ^

+ [([('Authorization', 'Bearer FOOBARBAZ'), ('User-agent', 'USER_AGENT')], None)]
?      ^                                      ^

Can you please update the PR accordingly?

@dhermes, @daspecster For future reference, here is the HTTP/2 spec reference which mandates lower-case for header field names (I was completely unaware of that requirement).

@tseaver
Copy link
Contributor

tseaver commented Aug 27, 2016

I cancelled the not-yet-started Travis build after verifying that it would fail.

@yang-g
Copy link
Contributor Author

yang-g commented Aug 27, 2016

@tseaver I pushed eebb83a about the same time of your comment. I did a search and did not find anything else. Could you restart the travis-ci or point me to more places to fix? Thanks.

@tseaver
Copy link
Contributor

tseaver commented Aug 27, 2016

@yang-g I restarted the build. Sorry for my haste in cancelling -- Travis has been so glacially slow lately that we've been minimizing "known broken" jobs in the queue.

@dhermes
Copy link
Contributor

dhermes commented Aug 27, 2016

@tseaver I think Travis have sorted out their issues

@tseaver tseaver merged commit a5594a7 into googleapis:master Aug 27, 2016
@tseaver
Copy link
Contributor

tseaver commented Aug 27, 2016

@yang-g Thank you again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement. grpc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants