Small improvements to the kudo init command and docs #880
Conversation
| @@ -32,6 +31,9 @@ version. You can specify an alternative image with '--kudo-image' which is the f | |||
| or '--version' which will replace the version designation on the standard image. | |||
|
|
|||
| To dump a manifest containing the KUDO deployment YAML, combine the '--dry-run' and '--output=yaml' flags. | |||
|
|
|||
| Running 'kudo init' on server-side is idempotent - it skips manifests alredy applied to the cluster in previous runs | |||
| and finishes with success if KUDO is already installed. | |||
There was a problem hiding this comment.
I'm not sure if necessary or not... but an important part of this message to me is that if the k8s objects exists... the create is skipped... there is no update. If kudo init was applied with version 0.7.0 followed by a future 0.8.0 there is no update to the objects. It would have to be uninstalled reinstalled currently.
There was a problem hiding this comment.
hmm, interesting, I actually did not know that so maybe that should be in the documentation as well 🤔
| @@ -168,10 +175,6 @@ func (initCmd *initCmd) run() error { | |||
| } | |||
|
|
|||
| if err := cmdInit.Install(initCmd.client, opts, initCmd.crdOnly); err != nil { | |||
| if apierrors.IsAlreadyExists(err) { | |||
There was a problem hiding this comment.
this removed message is still necessary IMO... unless we want to change line 108 of manager.go.
Most of the installs return nil if isAlreadyExistsError returns true... with the exception of the last install which is the kudo controller itself. If the kudo controller exists then it returns the error and this warning message was printed.
For clarity: this is what most install services do:
kudo/pkg/kudoctl/cmd/init/manager.go
Line 98 in 528bab0
This is what install the service does:
kudo/pkg/kudoctl/cmd/init/manager.go
Lines 106 to 111 in 528bab0
I'm marking as reject because we either:
- need to return nil from the install of the statefulset (along with the removal of this warning message) or
- need to return warning message.
There was a problem hiding this comment.
Interesting, I wonder what to do here. I don't really have any immediate answer, have to think about it...
but that means you cannot run init twice because the second one will always fail because the manager is already running, right?
I am actually starting to think that the correct behavior of init here is to do an update when the thing already exists. Because manager is not the only thing that if it exists, it means it is in the same state as what we're trying to apply 🤔 Let me know what you think... I think that way, it would be truly idempotent which would be lovely...
|
for public record... in slack channel we discussed the value and challenge of "update"... which is to say if the object exists to update to the latest version. The issue is there could be running operators which don't work with the newer version and we may need to migrate from one ver to another. Our current thinking for this PR is to do nothing if exists and to install if it doesn't exists. frankly this could have issues as well as we could have init'ed originally with Created this issue for tracking the future: #891 |
What this PR does / why we need it:
This was the main motivation for this PR https://github.com/kudobuilder/kudo/compare/av/init-small-improvements?expand=1#diff-c74faa6263971005d609e60d78ff0f8eL171 I think the message is no longer true and we should be skipping all already exists errors now, correct? The rest is just small wording improvements.