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

@W-11993356 Metecho: handle scratch org DNS propagation delays #2075

Open
wants to merge 20 commits into
base: main
Choose a base branch
from

Conversation

jofsky
Copy link
Contributor

@jofsky jofsky commented Nov 15, 2022

No description provided.

@jofsky jofsky changed the title Feature/dns propagation delay Metecho: handle scratch org DNS propagation delays Nov 15, 2022
@jofsky jofsky changed the title Metecho: handle scratch org DNS propagation delays @W-11993356 Metecho: handle scratch org DNS propagation delays Nov 16, 2022
@jofsky jofsky marked this pull request as ready for review November 16, 2022 00:16
@jofsky jofsky requested a review from a team as a code owner November 16, 2022 00:16
@jofsky jofsky requested a review from prescod November 16, 2022 00:17
Copy link
Contributor

@prescod prescod left a comment

Choose a reason for hiding this comment

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

I'm not a big fan of formatting changes being mixed into the same PR as logic changes.

Are you using a different version of black than the one that was used the last time these files were touched?

Other changes inline.

Comment on lines 328 to 330
if total_wait_time >= settings.MAXIMUM_JOB_LENGTH:
raise ScratchOrgError(
f"Failed to build your scratch org after {settings.MAXIMUM_JOB_LENGTH} seconds."
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if total_wait_time >= settings.MAXIMUM_JOB_LENGTH:
raise ScratchOrgError(
f"Failed to build your scratch org after {settings.MAXIMUM_JOB_LENGTH} seconds."
assert total_wait_time >= settings.MAXIMUM_JOB_LENGTH:
raise ScratchOrgError(
f"Failed to build your scratch org after {settings.MAXIMUM_JOB_LENGTH} seconds."

@@ -22,5 +22,6 @@ pytest-factoryboy
pytest-lazy-fixture
pytest-mock
pytest-sugar
responses
Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't realize that the project wasn't already using responses.

We should decide if we are actually planning to use responses more generally. Adding it to the project trigger a single exception is probably overkill. One line of mocking could achieve the same thing.

Adding the one line of mocking is probably clearer anyways.

Let's just find out what Requests code is calling the Responses code and throw an exception in it.

Copy link
Contributor

@prescod prescod left a comment

Choose a reason for hiding this comment

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

I could approve as-is but if we have time to make these changes then let's do it.

@prescod prescod force-pushed the feature/dns-propagation-delay branch from 76a1a18 to 006859d Compare November 21, 2022 20:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants