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 transport module for GCE environment check. #612

Merged
merged 1 commit into from Aug 16, 2016

Conversation

Projects
None yet
4 participants
@dhermes
Contributor

dhermes commented Aug 11, 2016

Fixes #599.

_GCE_METADATA_HOST = '169.254.169.254'
_METADATA_FLAVOR_HEADER = 'Metadata-Flavor'
_GCE_METADATA_URI = 'http://169.254.169.254'
_METADATA_FLAVOR_HEADER = 'metadata-flavor' # lowercase header

This comment has been minimized.

@nathanielmanistaatgoogle

nathanielmanistaatgoogle Aug 11, 2016

Contributor

I don't understand the significance of # lowercase header on this line.

@nathanielmanistaatgoogle

nathanielmanistaatgoogle Aug 11, 2016

Contributor

I don't understand the significance of # lowercase header on this line.

This comment has been minimized.

@nathanielmanistaatgoogle

nathanielmanistaatgoogle Aug 11, 2016

Contributor

How essential is "_HEADER" in this name?

@nathanielmanistaatgoogle

nathanielmanistaatgoogle Aug 11, 2016

Contributor

How essential is "_HEADER" in this name?

This comment has been minimized.

@dhermes

dhermes Aug 11, 2016

Contributor

the significance of # lowercase header

The previous value broke httplib2 (headers in httplib2 are all lowercased so avoid allowing case-insensitive header lookups)

How essential is "_HEADER" in this name?

Making the name shorter just to be short is a loss for code readability. The name describes what the variable is.

@dhermes

dhermes Aug 11, 2016

Contributor

the significance of # lowercase header

The previous value broke httplib2 (headers in httplib2 are all lowercased so avoid allowing case-insensitive header lookups)

How essential is "_HEADER" in this name?

Making the name shorter just to be short is a loss for code readability. The name describes what the variable is.

_GCE_METADATA_HOST = '169.254.169.254'
_METADATA_FLAVOR_HEADER = 'Metadata-Flavor'
_GCE_METADATA_URI = 'http://169.254.169.254'
_METADATA_FLAVOR_HEADER = 'metadata-flavor' # lowercase header
_DESIRED_METADATA_FLAVOR = 'Google'

This comment has been minimized.

@nathanielmanistaatgoogle

nathanielmanistaatgoogle Aug 11, 2016

Contributor

How essential is "_METADATA" in this name?

@nathanielmanistaatgoogle

nathanielmanistaatgoogle Aug 11, 2016

Contributor

How essential is "_METADATA" in this name?

This comment has been minimized.

@dhermes

dhermes Aug 11, 2016

Contributor

Ditto on my answer above

@dhermes

dhermes Aug 11, 2016

Contributor

Ditto on my answer above

@dhermes

This comment has been minimized.

Show comment
Hide comment
@dhermes

dhermes Aug 11, 2016

Contributor

@nathanielmanistaatgoogle PTAL I pushed a 2nd commit with some of the changes you suggest (see my responses for the changes I didn't make).

I also removed a no-longer relevant TODO (about separation from google-api-python-client) and the __author__ from the test_client file. (I did this because I noticed them when adding a module level constant in test_client.)

Contributor

dhermes commented Aug 11, 2016

@nathanielmanistaatgoogle PTAL I pushed a 2nd commit with some of the changes you suggest (see my responses for the changes I didn't make).

I also removed a no-longer relevant TODO (about separation from google-api-python-client) and the __author__ from the test_client file. (I did this because I noticed them when adding a module level constant in test_client.)

connection = six.moves.http_client.HTTPConnection(
_GCE_METADATA_HOST, timeout=GCE_METADATA_TIMEOUT)
http = transport.get_http_object(timeout=GCE_METADATA_TIMEOUT)

This comment has been minimized.

@theacodes

theacodes Aug 11, 2016

Member

Is it not possible to have timeout be an argument on request?

@theacodes

theacodes Aug 11, 2016

Member

Is it not possible to have timeout be an argument on request?

This comment has been minimized.

@dhermes

dhermes Aug 11, 2016

Contributor

It is "possible", though I'm not sure how it would work with existing httplib2.Http objects passed in.

We could file an issue and then follow up after / during redesign? (Now that the quarantine is essentially done.)

@dhermes

dhermes Aug 11, 2016

Contributor

It is "possible", though I'm not sure how it would work with existing httplib2.Http objects passed in.

We could file an issue and then follow up after / during redesign? (Now that the quarantine is essentially done.)

This comment has been minimized.

@theacodes

theacodes Aug 11, 2016

Member

I would greatly prefer that, as when we switch to urllib3 we will want to use a single PoolManager instead of individual http instances.

@theacodes

theacodes Aug 11, 2016

Member

I would greatly prefer that, as when we switch to urllib3 we will want to use a single PoolManager instead of individual http instances.

This comment has been minimized.

@dhermes

dhermes Aug 11, 2016

Contributor

Filed #618 for now.

@dhermes

dhermes Aug 11, 2016

Contributor

Filed #618 for now.

@theacodes

This comment has been minimized.

Show comment
Hide comment
@theacodes

theacodes Aug 11, 2016

Member

LGTM for now.

Member

theacodes commented Aug 11, 2016

LGTM for now.

@dhermes

This comment has been minimized.

Show comment
Hide comment
@dhermes

dhermes Aug 11, 2016

Contributor

@jonparrott Can you think of any reason that this wouldn't have been the original implementation?

@nathanielmanistaatgoogle Any issues?

Contributor

dhermes commented Aug 11, 2016

@jonparrott Can you think of any reason that this wouldn't have been the original implementation?

@nathanielmanistaatgoogle Any issues?

@theacodes

This comment has been minimized.

Show comment
Hide comment
@theacodes

theacodes Aug 11, 2016

Member

I'm not sure, I didn't write the original environment check. /shruggie

Member

theacodes commented Aug 11, 2016

I'm not sure, I didn't write the original environment check. /shruggie

@dhermes

This comment has been minimized.

Show comment
Hide comment
@dhermes

dhermes Aug 11, 2016

Contributor

Got heem

Contributor

dhermes commented Aug 11, 2016

Got heem

@dhermes

This comment has been minimized.

Show comment
Hide comment
@dhermes

dhermes Aug 15, 2016

Contributor

@nathanielmanistaatgoogle PTAL. Pretty please remind me to squash in your LGTM. (@jonparrott is there no such thing as a squash-bot / remind-bot service for GitHub?)

Contributor

dhermes commented Aug 15, 2016

@nathanielmanistaatgoogle PTAL. Pretty please remind me to squash in your LGTM. (@jonparrott is there no such thing as a squash-bot / remind-bot service for GitHub?)

@theacodes

This comment has been minimized.

Show comment
Hide comment
@theacodes

theacodes Aug 15, 2016

Member

@dhermes what would you want it to do?

Member

theacodes commented Aug 15, 2016

@dhermes what would you want it to do?

@dhermes

This comment has been minimized.

Show comment
Hide comment
@dhermes

dhermes Aug 15, 2016

Contributor

Block a merge until commits with the word SQUASH are gone? Or until a squash label is removed.

Contributor

dhermes commented Aug 15, 2016

Block a merge until commits with the word SQUASH are gone? Or until a squash label is removed.

@theacodes

This comment has been minimized.

Show comment
Hide comment
@theacodes

theacodes Aug 15, 2016

Member

I could probably add that to @dpebot once he's functional.

Member

theacodes commented Aug 15, 2016

I could probably add that to @dpebot once he's functional.

@dhermes

This comment has been minimized.

Show comment
Hide comment
@dhermes

dhermes Aug 15, 2016

Contributor

Ha awesome!

Contributor

dhermes commented Aug 15, 2016

Ha awesome!

@nathanielmanistaatgoogle

This comment has been minimized.

Show comment
Hide comment
@nathanielmanistaatgoogle

nathanielmanistaatgoogle Aug 16, 2016

Contributor

Change content looks good; squash and merge.

Contributor

nathanielmanistaatgoogle commented Aug 16, 2016

Change content looks good; squash and merge.

@dhermes dhermes merged commit 4c7b3be into googleapis:master Aug 16, 2016

3 checks passed

cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 100.0%
Details

@dhermes dhermes deleted the dhermes:fix-599 branch Aug 16, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment