Add help message when starting multiple machines #23863
Conversation
|
Ephemeral COPR build failed. @containers/packit-build please check. |
pkg/machine/shim/host.go
Outdated
| @@ -283,6 +283,7 @@ func checkExclusiveActiveVM(provider vmconfigs.VMProvider, mc *vmconfigs.Machine | |||
| return err | |||
| } | |||
| if state == machineDefine.Running || state == machineDefine.Starting { | |||
| fmt.Printf("Cannot start \"%s\" if machine \"%s\" is running:\n\n\tpodman machine stop %s\n\tpodman machine start %s\n\n", mc.Name, name, name, mc.Name) | |||
There was a problem hiding this comment.
I really dislike these random prints in "backend" code. I think we should just switch the error message to include the extra info.
There was a problem hiding this comment.
only reason why I preferred this over detailed err message is because we already hint to start the machine when we just do init so this is just another minor troubleshooting. but I also realize that for init is just hinting the next step while here it's suggesting a fix, and that's quite different.
There was a problem hiding this comment.
maybe the error should say something like this?
unable to start "vmY": machine "vmX" is already running or starting and there can only be one running machine at a time
There was a problem hiding this comment.
oh actually, we have an error for that which is ErrMultipleActiveVM just noticed now, is it a breaking change to change the error? I believe it makes more sense to use that
There was a problem hiding this comment.
Error messages are not stable so we can change them, for the error types you should check if callers do some errors.Is()/As() checks on them, if so you need to update the callers but there are not stable either as we do not offer stable APIs except for packages in pkg/bindings.
There was a problem hiding this comment.
thanks a lot. I'll check and update the PR
|
Gosh I was so unsure of what was the right keyword. I'll change it 😓 |
inknos
force-pushed
the
issue-23436
branch
3 times, most recently
from
cfdde34
to
71b308d
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: inknos, Luap99 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
LGTM |
Instead of ErrVMAlreadyRunning use a more appropriate error. Also improve the message a little bit. Fixes: containers#23436 Signed-off-by: Nicola Sella <[email protected]>
|
/lgtm |
openshift-merge-bot
bot
merged commit fdb8b1c
into
containers:main
Not much can be done when multiple machines are started but a help message is definitely of help in some instances
Resolves: #23436
Does this PR introduce a user-facing change?