-
Notifications
You must be signed in to change notification settings - Fork 103
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
kudoinit refactoring #1491
kudoinit refactoring #1491
Conversation
Signed-off-by: Andreas Neumann <[email protected]>
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.
This is great, thanks for adding a distinction between these different kinds of error sources. Looks like a lot of API errors were swallowed in the old code. Only have some minor regards.
|
||
// TODO: Add verification of existing installation | ||
// VerifyInstallation(client *kube.Client) Result | ||
PreInstallVerify(client *kube.Client, result *verifier.Result) 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.
How about returning (verifier.Result, error)
instead? This way there would be a clear distinction between input and output values. Also, currently most of the times when PreInstallVerify
is called we have something like this:
result := verifier.NewResult()
err = crd.NewInitializer().VerifyInstallation(kubeClient, &result)
whereas
result, err := crd.NewInitializer().VerifyInstallation(kubeClient)
is much clearer IMO. For the code in crds.go
to merges multiple Result
s, we could then use result.Merge
. WDYT?
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.
Yeah... I'm not sure, to be honest. This was basically the pattern I had before, and I do agree it looks nicer on the caller side.
Passing the result in allow the handling inside the funcs to look like this though:
if err := o.validateServiceAccountExists(client, result); err != nil {
return err
}
If were to return (verifier.Result, error) it would have to look like this:
myRes := verifier.NewResult{}
res, err := o.validateServiceAccountExists(client)
if err != nil {
return err
}
myRes.merge(res)
// ... next check, validate SA permissions
return res
So... It might be an option to change the signature for the Top-Level methods, but that is at the cost of consistency.
If other devs feels strongly about this as well I'll change it, but at the moment I prefer it like this.
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.
Yeah, I see how having multiple return values complicates things a bit, we can't do
result.Merge(o.validateServiceAccountExists(client))
I don't feel strongly about this, happy to keep it as it is right now.
return nil, err | ||
} | ||
result := verifier.NewResult() | ||
err = crd.NewInitializer().VerifyInstallation(kubeClient, &result) |
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.
Hmm, the old code doesn't do that, but shouldn't we run VerifyInstallation
only if validateInstall == true
? Closely related: If we run VerifyInstallation
regardless of validateInstall
, we should always return its error if there is on, similar to how it's done in the old code.
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.
Well - generally we always want to run VerifyInstallation. The validateInstall
parameter is by default "true", and it allows a user to continue with the process even if the verification is returning errors. It's basically "I know what I'm doing, don't annoy me"-Button.
For returning an error - The new code behaves the same as the old one, as far as I can see: It aborts if the verification fails and the parameter is true, and continues if the parameter is false.
I have renamed the parameter though...
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.
The old code returned an error only for os.IsTimeout
, regardless of validateInstall
. IMO, we should do the same and always return an error if VerifyInstallation
returns one, because it indicates something wrong with the cluster in general. For warnings/errors related to the CRDs specifically, Result
is used and there validateInstall
decides if an error is reported. WDYT?
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.
Oooh, that's what you mean. Right. Yeah, that makes sense.
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, I don't have strong opinion on the comments raised above.
Signed-off-by: Andreas Neumann <[email protected]>
Signed-off-by: Andreas Neumann <[email protected]>
@@ -196,6 +205,20 @@ func (initCmd *initCmd) run() error { | |||
return nil | |||
} | |||
|
|||
// preInstallVerify runs the pre-installation verification and returns true if the installation can continue | |||
func (initCmd *initCmd) preInstallVerify(opts kudoinit.Options) (bool, 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.
I like this 👍
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!
Signed-off-by: Andreas Neumann <[email protected]>
Extracted refactoring of
kudoinit
from #1467-- The Verify-Methods now don't return a
verifier.Result
anymore, but get averifier.Result
as a parameter and return a normal error to be more in sync with normal go error handling. The Verify methods can either return normally ( and have a result with errors/warnings that describe issues with the installation) or abnormally ( then they return anerror
) if the verification failed due to unexpected issues (api-server not responding, etc.)-- The prerequisites (
namespace, service account, webhooks
, etc.) are now simplekudoinit.Steps
and not special extra subtypes.