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.

Sign up for GitHub

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

Jump to bottom

IonQ: Handle 409 errors that are injected by Cloudflare #5292

Merged
merged 6 commits into from
Feb 22, 2023
Merged

IonQ: Handle 409 errors that are injected by Cloudflare #5292

merged 6 commits into from
Feb 22, 2023

Conversation

Cynocracy
Copy link
Contributor

When these occur on GETs, they can be retried.

@CirqBot CirqBot added the size: S 10< lines changed <50 label Apr 26, 2022
dabacon
dabacon approved these changes Apr 26, 2022
View reviewed changes
Copy link
Collaborator

@dabacon dabacon left a comment

Choose a reason for hiding this comment

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

An excellent job not selling me on cloudfare.

cirq-ionq/cirq_ionq/ionq_client.py Outdated Show resolved Hide resolved
@Cynocracy
Copy link
Contributor Author

I do my best ;) These errors are hopefully going away, but we're pretty set on not returning 409s ourselves on GETs, so ty for the review!

@Cynocracy Cynocracy mentioned this pull request Feb 15, 2023
Merged
@Cynocracy Cynocracy force-pushed the cloudflare-what-the-heck branch from aecdc29 to 4f05fbd Compare February 15, 2023 19:52
@Cynocracy
Copy link
Contributor Author

Revived this zombie PR 🧟
Merged with #6006

@Cynocracy Cynocracy requested a review from dabacon February 15, 2023 19:53
@Cynocracy Cynocracy force-pushed the cloudflare-what-the-heck branch from 5db3bcd to 6474575 Compare February 15, 2023 21:45
@Cynocracy
Copy link
Contributor Author

test appears to have flaked

@Cynocracy
Copy link
Contributor Author

Are the typescript tests just busted? (also there's typescript now? 😮 )

@vtomole vtomole mentioned this pull request Feb 21, 2023
Merged
@vtomole
Copy link
Collaborator

vtomole commented Feb 21, 2023

@Cynocracy I have a fix here: #6014

@Cynocracy
Copy link
Contributor Author

Thanks Victory! This was actually a separate PR, was just noting the test failures

@ColemanCollins
Copy link
Collaborator

Reopening and updating from master; per jon the flaky tests were causing this to not be ready to merge but the underlying failure around cloudflare fronting the IonQ API still needs to be addressed

ColemanCollins
ColemanCollins reviewed Feb 22, 2023
View reviewed changes
cirq-ionq/cirq_ionq/ionq_client_test.py
response2.ok = True
response2.json.return_value = {'foo': 'bar'}

client = ionq.ionq_client._IonQClient(remote_host='http://example.com', api_key='to_my_heart')
Copy link
Collaborator

Choose a reason for hiding this comment

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

@dabacon @Cynocracy I still love the dummy API key we use for these tests

Copy link
Contributor Author

Choose a reason for hiding this comment

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

lol, it's pretty great

@ColemanCollins ColemanCollins merged commit 08fee59 into quantumlib:master Feb 22, 2023
@Cynocracy Cynocracy deleted the cloudflare-what-the-heck branch February 22, 2023 17:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ColemanCollins ColemanCollins ColemanCollins left review comments

@gmauricio gmauricio gmauricio approved these changes

@nakardo nakardo Awaiting requested review from nakardo

@vtomole vtomole Awaiting requested review from vtomole

@cduck cduck Awaiting requested review from cduck

@dabacon dabacon Awaiting requested review from dabacon

Assignees
No one assigned
Labels
size: S 10< lines changed <50
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Cynocracy @vtomole @ColemanCollins @dabacon @gmauricio @CirqBot