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

adding the --wait flag to the apps create command #994

Merged
merged 3 commits into from Jun 21, 2021
Merged

Conversation

danaelhe
Copy link
Contributor

@danaelhe danaelhe commented Jun 15, 2021

(reopened PR to disregard 400+ vendor files committed)

per github issue #970 - adding a wait flag for doctl apps create command. The integration test is taking 30 seconds to complete. This causes a timeout error- if timeout is removed it works. Looking for a second pair of eyes to help determine why the test takes so long.

applies changes suggested from previous pr such as:

  • replaced ticker with for loop (thanks @bentranter)
  • clearer error messages

(reopened PR to disregard 400+ vendor files committed)

per github issue #970 - adding a wait flag for doctl apps create command. The integration test is taking 30 seconds to complete. This causes a timeout error- if timeout is removed it works. Looking for a second pair of eyes to help determine why the test takes so long.

applies changes suggested from previous pr such as:
- replaced ticker with for loop (thanks @bentranter)
- clearer error messages
@danaelhe danaelhe requested a review from Jun 15, 2021
Copy link
Member

@andrewsomething andrewsomething left a comment

Looks great, but I did have one question/comment inline about the length of the timeout.

commands/apps.go Outdated
func waitForAppDeploymentRunning(apps do.AppsService, appID string, deploymentID string) (*godo.Deployment, error) {
failCount := 0
func waitForActiveDeployment(apps do.AppsService, appID string, deploymentID string) error {
const maxAttempts = 10 * 6 * 30
Copy link
Member

@andrewsomething andrewsomething Jun 15, 2021

Choose a reason for hiding this comment

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

Think we might want something a bit shorter here. 1800 attempts at 10 seconds each is 5 hours. Maybe something like 30 minutes?

Copy link
Contributor Author

@danaelhe danaelhe Jun 15, 2021

Choose a reason for hiding this comment

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

oh 30 minutes is more reasonable hahaha. Will do.

commands/apps.go Outdated Show resolved Hide resolved
@andrewsomething andrewsomething linked an issue Jun 16, 2021 that may be closed by this pull request
Copy link
Member

@andrewsomething andrewsomething left a comment

👍 LGTM!

@danaelhe danaelhe merged commit 318f8e0 into main Jun 21, 2021
8 checks passed
@ChiefMateStarbuck ChiefMateStarbuck deleted the add_wait_to_apps branch Mar 7, 2022
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.

2 participants