-
Notifications
You must be signed in to change notification settings - Fork 133
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
deploy: refactor getResource to return NotFound for both cases #1046
deploy: refactor getResource to return NotFound for both cases #1046
Conversation
pkg/deploy/deploy.go
Outdated
// Create resource if it doesn't exist | ||
if found == nil { | ||
return createResource(ctx, cli, obj, owner) | ||
if err == nil { |
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.
should this be if found == nil
?
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've exchanged createResource/updateResource to avoid found == nil check
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.
sorry, meant to say
if found != nil { // do update }
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 will work as well, but if there was no error (err == nil), found is not nil in getResource() so we can stay on checking err only
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 could be more clear actually:
func manageResource(ctx context.Context, cli client.Client, obj *unstructured.Unstructured, owner metav1.Object, applicationNamespace, componentName string, enabled bool) error {
// Return if resource is of Kind: Namespace and Name: odhApplicationsNamespace
if obj.GetKind() == "Namespace" && obj.GetName() == applicationNamespace {
return nil
}
found, err := getResource(ctx, cli, obj)
// err == nil means found
if err == nil {
if enabled {
return updateResource(ctx, cli, obj, found, owner, componentName)
}
return handleDisabledComponent(ctx, cli, found, componentName)
}
if apierrs.IsNotFound(err) {
// Create resource if it doesn't exist
return createResource(ctx, cli, obj, owner)
}
return err
}
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,
Maybe simplify it more as you suggest in https://github.com/opendatahub-io/opendatahub-operator/pull/1046/files#r1632844023
b5ab7c4
to
78d1b32
Compare
Avoid returning (nil, nil), convert error if CRD not present to NotFound and handle them in the caller code. Avoid checking for found == nil but handle only error code. This is more idiomatic and less error prone. Signed-off-by: Yauheni Kaliuta <[email protected]>
78d1b32
to
974218f
Compare
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.
Thanks for the PR @ykaliuta
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bartoszmajsak, VaishnaviHire, zdtsw 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 |
9651b76
into
opendatahub-io:incubation
…atahub-io#1046) Avoid returning (nil, nil), convert error if CRD not present to NotFound and handle them in the caller code. Avoid checking for found == nil but handle only error code. This is more idiomatic and less error prone. Signed-off-by: Yauheni Kaliuta <[email protected]> (cherry picked from commit 9651b76)
- since we already have these resource for create, list, delete etc, no harm to add get on them Signed-off-by: Wen Zhou <[email protected]> (cherry picked from commit bd9b83c) fix: when delete resource if no CRD in the cluster (opendatahub-io#1044) Signed-off-by: Wen Zhou <[email protected]> (cherry picked from commit c040f77) deploy: refactor getResource to return NotFound for both cases (opendatahub-io#1046) Avoid returning (nil, nil), convert error if CRD not present to NotFound and handle them in the caller code. Avoid checking for found == nil but handle only error code. This is more idiomatic and less error prone. Signed-off-by: Yauheni Kaliuta <[email protected]> (cherry picked from commit 9651b76) Handle Kserve exception Fix csv
- since we already have these resource for create, list, delete etc, no harm to add get on them Signed-off-by: Wen Zhou <[email protected]> (cherry picked from commit bd9b83c) fix: when delete resource if no CRD in the cluster (opendatahub-io#1044) Signed-off-by: Wen Zhou <[email protected]> (cherry picked from commit c040f77) deploy: refactor getResource to return NotFound for both cases (opendatahub-io#1046) Avoid returning (nil, nil), convert error if CRD not present to NotFound and handle them in the caller code. Avoid checking for found == nil but handle only error code. This is more idiomatic and less error prone. Signed-off-by: Yauheni Kaliuta <[email protected]> (cherry picked from commit 9651b76) Handle Kserve exception Fix csv (cherry picked from commit e5f65bd)
…atahub-io#1046) Avoid returning (nil, nil), convert error if CRD not present to NotFound and handle them in the caller code. Avoid checking for found == nil but handle only error code. This is more idiomatic and less error prone. Signed-off-by: Yauheni Kaliuta <[email protected]>
Avoid returning (nil, nil), convert error if CRD not present to NotFound and handle them in the caller code.
Avoid checking for found == nil but handle only error code.
This is more idiomatic and less error prone.
Description
ref: https://issues.redhat.com/browse/RHOAIENG-8297
How Has This Been Tested?
Merge criteria: