-
Notifications
You must be signed in to change notification settings - Fork 34
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
Conversation
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.
Some comments
pkg/cgu/precachingconfig.go
Outdated
if name == "" { | ||
glog.V(100).Infof("The name of the PreCachingConfig is empty") | ||
|
||
builder.errorMsg = "preCachingConfig 'name' cannot be empty" | ||
} |
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.
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 | |
} |
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.
Per #355
pkg/cgu/precachingconfig.go
Outdated
if nsname == "" { | ||
glog.V(100).Infof("The namespace of the PreCachingConfig is empty") | ||
|
||
builder.errorMsg = "preCachingConfig 'nsname' cannot be empty" | ||
} |
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.
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 | |
} |
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.
Per #355
pkg/cgu/precachingconfig.go
Outdated
} | ||
|
||
if !builder.Exists() { | ||
return nil, fmt.Errorf("preCachingConfig object %s doesn't exist in namespace %s", name, nsname) |
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.
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) |
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.
Per #390
pkg/cgu/precachingconfig_test.go
Outdated
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. |
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 update won't fail. | |
// the update will not fail. |
pkg/cgu/precachingconfig_test.go
Outdated
addToRuntimeObjects: false, | ||
client: true, | ||
expectedErrorText: fmt.Sprintf( | ||
"preCachingConfig object %s doesn't exist in namespace %s", |
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.
"preCachingConfig object %s doesn't exist in namespace %s", | |
"preCachingConfig object %s does not exist in namespace %s", |
@sebrandon1 Thanks, I've updated the PR with changes |
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
Approved! I don't have the power to resolve my comments above so that will have to be someone else. |
No description provided.