🌱 add missing unit tests for the pathannotation admission plugin #2535
Conversation
openshift-ci
bot
added
the
kind/api-change
|
/lgtm |
| expectError: true, | ||
| }, | ||
| { | ||
| name: "admission is not applied to logicalclusters", // would panic since no admission obj |
There was a problem hiding this comment.
What does the comment mean? It would panic, but it doesn't, and why. ..?
There was a problem hiding this comment.
I was lazy - instead of providing an object and making sure it doesn't have the annotation set i assumed it would panic if the admission would apply. Anyway I added an object and provided objectWithoutPathAnnotation function.
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: corev1alpha1.LogicalClusterName, | ||
| Annotations: map[string]string{ | ||
| core.LogicalClusterPathAnnotationKey: "root:foo", |
There was a problem hiding this comment.
Why is this root when the clusterName passed in is test-cluster?
There was a problem hiding this comment.
changed the clusterName to foo
| admissionVerb: admission.Create, | ||
| admissionResource: apisv1alpha1.SchemeGroupVersion.WithResource("apiexports"), | ||
| admissionObject: &apisv1alpha1.APIExport{}, | ||
| admissionContext: wellKnownAdmissionContext, |
There was a problem hiding this comment.
nit: factoring out the data that drives the test would make this easier to read (and explicit, not implicit):
{
name: "happy path: an APIExport is annotated with a path",
admissionVerb: admission.Create,
admissionResource: apisv1alpha1.SchemeGroupVersion.WithResource("apiexports"),
admissionObject: &apisv1alpha1.APIExport{},
admissionContext: admissionContextFor("root:foo"),
getLogicalCluster: getCluster("root:foo"),
validateAdmissionObject: objectHasPathAnnotation("root:foo"),
},| @@ -101,20 +105,20 @@ func (p *pathAnnotationPlugin) Admit(ctx context.Context, a admission.Attributes | |||
| return nil | |||
| } | |||
|
|
|||
| this, err := p.logicalClusterLister.Cluster(clusterName).Get(corev1alpha1.LogicalClusterName) | |||
| cluster, err := p.getLogicalCluster(clusterName, corev1alpha1.LogicalClusterName) | |||
There was a problem hiding this comment.
super-nit: can you make these logicalCluster please? xref #2524
There was a problem hiding this comment.
it looks like you already did it in another PR :)
p0lyn0mial
force-pushed
the
path-annotation-ut
branch
from
686f318
to
8cf5c0e
Compare
openshift-merge-robot
added
the
needs-rebase
p0lyn0mial
force-pushed
the
path-annotation-ut
branch
from
8cf5c0e
to
82504cf
Compare
openshift-merge-robot
removed
the
needs-rebase
|
@s-urbaniak @stevekuznetsov i've updated the PR, PTAL. |
|
|
||
| // getLogicalCluster is a convenience function for easier unit testing, | ||
| // it reads a LogicalCluster resource with the given name and from the given cluster. | ||
| getLogicalCluster func(clusterName logicalcluster.Name, name string) (*corev1alpha1.LogicalCluster, error) |
|
/retest |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sttts 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 |
openshift-ci
bot
added
the
approved
|
/retest |
Related issue(s)
#2510