-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
Instance Running waiter not aware of global waiter settings #8699
Instance Running waiter not aware of global waiter settings #8699
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @fly1028, thanks ! LGTM
InstanceIds: []*string{&instanceId}, | ||
} | ||
|
||
err := conn.WaitUntilInstanceRunningWithContext( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh super nice, I wasn't aware of this one !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Seeing as we don’t actually use err here we can just return with the call to conn.WaitUntilInstanceRunningWithContext(...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nywilken I'm following the convention of other existing WaitUntil* methods in state.go
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fly1028 thanks I only looked at the diff. But this is good to go I just have not had a chance to merge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM as well!! Nice catch 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM as well. I left a small comment to drop the assignment of err for a return but that shouldn’t prevent the merge.
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
This is related to #6177.
The work done for #6177 didn't really address the hardcoded timeout issue for the run source instance step.
Specifically, it's not aware of the AWS_POLL_DELAY_SECONDS and AWS_TIMEOUT_SECONDS.