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: add retry to google.auth.compute_engine._metadata.get() #398

Conversation

DaniilAnichin
Copy link
Contributor

Initial fix of issue #211 was done in CL #323, but only for .ping()
This one is adding same behavior & tests for .get() method, as the problem still arises

Resolves: #211

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Dec 5, 2019
@DaniilAnichin DaniilAnichin force-pushed the feature/211-add-retry-on-metadata-get branch 2 times, most recently from e0c2dbd to 917df1a Compare December 5, 2019 15:55
@DaniilAnichin
Copy link
Contributor Author

@dhermes can you take a look to finally make the #211 fixed?

@dhermes
Copy link
Contributor

dhermes commented Dec 5, 2019

cc @busunkim96

@DaniilAnichin I am no longer maintaining this library. Cheers

@DaniilAnichin DaniilAnichin force-pushed the feature/211-add-retry-on-metadata-get branch from 917df1a to 1226931 Compare December 19, 2019 14:35
@DaniilAnichin DaniilAnichin force-pushed the feature/211-add-retry-on-metadata-get branch from 1226931 to 2f75de9 Compare December 26, 2019 15:42
@lucasmbrown
Copy link

I can't tell who the maintainers currently are, but @theacodes or @busunkim96, would you be available to review this? It's caused us a few headaches so far (right now we're wrapping all our gets in our own retry logic). Thanks!

@busunkim96
Copy link
Contributor

@lucasmbrown Thank you for the ping! I missed this PR. I'll take a look now.

@busunkim96 busunkim96 added the kokoro:run Add this label to force Kokoro to re-run the tests. label Jan 8, 2020
@busunkim96 busunkim96 self-requested a review January 8, 2020 19:24
@yoshi-kokoro yoshi-kokoro removed the kokoro:run Add this label to force Kokoro to re-run the tests. label Jan 8, 2020
Copy link
Contributor

@busunkim96 busunkim96 left a comment

Choose a reason for hiding this comment

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

It looks like there's a mismatch between the actual retry count (5) and the count in the test (3). Otherwise this looks good to me. Thank you for the PR!

=================================== FAILURES ===================================
______________________ test_get_failure_connection_failed ______________________

    def test_get_failure_connection_failed():
        request = make_request("")
        request.side_effect = exceptions.TransportError()

        with pytest.raises(exceptions.TransportError) as excinfo:
            _metadata.get(request, PATH)

        assert excinfo.match(r"Compute Engine Metadata server unavailable")

        request.assert_called_with(
            method="GET",
            url=_metadata._METADATA_ROOT + PATH,
            headers=_metadata._METADATA_HEADERS,
        )
>       assert request.call_count == 3
E       AssertionError: assert 5 == 3
E        +  where 5 = <MagicMock spec='Request' id='140641503619536'>.call_count

tests/compute_engine/test__metadata.py:207: AssertionError

setup.py Outdated Show resolved Hide resolved
…mpute_engine._metadata.get()

Initial fix of issue googleapis#211 was done in CL googleapis#323, but only for .ping()
This one is adding same behaviour & tests for .get() method, as the problem still occurres
See the issue for details

Refs: googleapis#323
Resolves: googleapis#211
@DaniilAnichin
Copy link
Contributor Author

Updated call count in tests; ping me if I need to rebase to current master

@DaniilAnichin DaniilAnichin force-pushed the feature/211-add-retry-on-metadata-get branch from 2f75de9 to 3a1187f Compare January 9, 2020 13:52
@busunkim96 busunkim96 added the kokoro:run Add this label to force Kokoro to re-run the tests. label Jan 9, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:run Add this label to force Kokoro to re-run the tests. label Jan 9, 2020
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Intermittent DefaultCredentialsError on GCE
6 participants