Skip to content
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

cgu: added PreCachingConfig resource and unit tests #389

Merged
merged 1 commit into from
May 3, 2024

Conversation

klaskosk
Copy link
Collaborator

@klaskosk klaskosk commented May 2, 2024

No description provided.

Copy link
Collaborator

@sebrandon1 sebrandon1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some comments

Comment on lines 43 to 49
if name == "" {
glog.V(100).Infof("The name of the PreCachingConfig is empty")

builder.errorMsg = "preCachingConfig 'name' cannot be empty"
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if name == "" {
glog.V(100).Infof("The name of the PreCachingConfig is empty")
builder.errorMsg = "preCachingConfig 'name' cannot be empty"
}
if name == "" {
glog.V(100).Infof("The name of the PreCachingConfig is empty")
builder.errorMsg = "preCachingConfig 'name' cannot be empty"
return builder
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per #355

Comment on lines 49 to 57
if nsname == "" {
glog.V(100).Infof("The namespace of the PreCachingConfig is empty")

builder.errorMsg = "preCachingConfig 'nsname' cannot be empty"
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if nsname == "" {
glog.V(100).Infof("The namespace of the PreCachingConfig is empty")
builder.errorMsg = "preCachingConfig 'nsname' cannot be empty"
}
if nsname == "" {
glog.V(100).Infof("The namespace of the PreCachingConfig is empty")
builder.errorMsg = "preCachingConfig 'nsname' cannot be empty"
return builder
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per #355

}

if !builder.Exists() {
return nil, fmt.Errorf("preCachingConfig object %s doesn't exist in namespace %s", name, nsname)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return nil, fmt.Errorf("preCachingConfig object %s doesn't exist in namespace %s", name, nsname)
return nil, fmt.Errorf("preCachingConfig object %s does not exist in namespace %s", name, nsname)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per #390

testBuilder := buildValidPreCachingConfigTestBuilder(clients.GetTestClients(clients.TestClientParams{}))

// Create the builder rather than just adding it to the client so that the proper metadata is added and
// the update won't fail.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// the update won't fail.
// the update will not fail.

addToRuntimeObjects: false,
client: true,
expectedErrorText: fmt.Sprintf(
"preCachingConfig object %s doesn't exist in namespace %s",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"preCachingConfig object %s doesn't exist in namespace %s",
"preCachingConfig object %s does not exist in namespace %s",

@klaskosk klaskosk force-pushed the precachingconfig branch from 42c7be0 to abcb7f3 Compare May 2, 2024 19:19
@klaskosk
Copy link
Collaborator Author

klaskosk commented May 2, 2024

@sebrandon1 Thanks, I've updated the PR with changes

Copy link
Collaborator

@kononovn kononovn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@sebrandon1
Copy link
Collaborator

Approved! I don't have the power to resolve my comments above so that will have to be someone else.

@kononovn kononovn merged commit 08fcad3 into openshift-kni:main May 3, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants