-
Notifications
You must be signed in to change notification settings - Fork 75
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
Add wait flag to sites commands #1795
base: v2
Are you sure you want to change the base?
Conversation
isStatusReached := false | ||
|
||
switch cmd.status { | ||
case "pending": |
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.
I don't think 'pending' is a state you wait for. It is an indication that no state has been settled yet. One option is to wait until a 'configured' condition has been recorded, whether true or false (if false there is likely to be an error).
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.
That makes sense. The wait flag also allows to wait for configured
, but following the Refdog documentation (https://skupperproject.github.io/refdog/commands/site/create.html), when a user indicates --wait pending
it could mean that they don't expect to wait at all, right? so in that case I just need to skip the WaitUntil step. Do you agree with that?
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.
I'm disagreeing with the refdog documentation in that I don't think 'pending' should be one of the options to wait. @ssorj
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.
I have removed the pending
value (d917edf) from the possible values to include in the --wait
flag, in order to unblock this pull request until we find a consensus.
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.
Looks fine to me. One question: what do we want the behaviour to be if something goes wrong with the configuration (i.e. there is an error)? Do we want to just timeout, or do we want to react to that error condition and exit with the error?
internal/cmd/skupper/common/flags.go
Outdated
@@ -70,6 +70,10 @@ for other Kubernetes flavors, loadbalancer is the default.` | |||
FlagDescListenerPort = "The port of the local listener" | |||
FlagNameListenerHost = "host" | |||
FlagDescListenerHost = "The hostname or IP address of the local listener. Clients at this site use the listener host and port to establish connections to the remote service." | |||
|
|||
FlagNameWait = "wait" | |||
FlagDescWait = "Wait for the given status before exiting. Choices: pending, configured, ready" |
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.
As per the latest discussion, I believe we need to remove "pending" from the description.
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.
Replaced by none
, as discussed.
…us was reached with errors
If the site reaches the selected status with error, the message indicates that and exit with an error. If the status was not reached by any means, it exits with error as well. |
Description of the changes:
Wait for a given status
New flag that allows the user to wait for a status (pending, configured, ready).
skupper site create
andskupper site update
skupper site create my-site --wait configured
Wait for the site to be deleted
New flag tat allows the user to wait or not for the site to be deleted.
skupper site delete
skupper site delete --wait=false
These flags are available only for Kubernetes platforms for now.