-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
🐛 Fix metadata assert failure in clusterclass rollout test #10840
🐛 Fix metadata assert failure in clusterclass rollout test #10840
Conversation
Add union() to convert a potential nil map to an empty map, otherwise the test will faile due to compare a nil map and an empty one.
794e3a5
to
dc7ff5d
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.
/lgtm
/approve
LGTM label has been added. Git tree hash: ad1b18411a967488c77f5ab59e70edb0873b49b1
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fabriziopandini 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 |
@@ -407,7 +407,7 @@ func assertControlPlane(g Gomega, clusterClassObjects clusterClassObjects, clust | |||
ccControlPlaneTemplateMachineTemplateMetadata.Labels, | |||
), | |||
)) | |||
g.Expect(controlPlaneMachineTemplateMetadata.Annotations).To(BeEquivalentTo( | |||
g.Expect(union(controlPlaneMachineTemplateMetadata.Annotations)).To(BeEquivalentTo( |
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.
What is the pattern behind adding this to in some places but not in others?
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.
Hey @sbueringer , I only added union() for the cases which are more possible to compare a nil
and empty
map, when only have union() in one of Expect
or BeEquivalentTo
as below
g.Expect(machineDeployment.Spec.Template.Annotations).To(BeEquivalentTo(
union(
mdTopology.Metadata.Annotations,
mdClass.Template.Metadata.Annotations,
),
))
I think other cases are less likely to run into this situation, such as
g.Expect(infrastructureMachineTemplate.GetAnnotations()).To(BeEquivalentTo(
union(
map[string]string{
clusterv1.TemplateClonedFromGroupKindAnnotation: groupKind(mdClass.Template.Infrastructure.Ref),
clusterv1.TemplateClonedFromNameAnnotation: mdClass.Template.Infrastructure.Ref.Name,
},
ccInfrastructureMachineTemplate.GetAnnotations(), ===> We defined expected annotations, and if infrastructureMachineTemplate.GetAnnotations() are nil or empty, that's a failure we should catch.
),
))
// MachineDeployment InfrastructureMachineTemplate.spec.template.metadata
g.Expect(infrastructureMachineTemplateTemplateMetadata.Labels).To(BeEquivalentTo(
ccInfrastructureMachineTemplateTemplateMetadata.Labels, ===> Usually they either have labels defined or are 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.
Hm. I think the following might be a better option.
func expectMapsToBeEquivalent(g Gomega, m1, m2 map[string]string) {
if m1 == nil {
m1 = map[string]string{}
}
if m2 == nil {
m2 = map[string]string{}
}
g.ExpectWithOffset(1, m1).To(BeEquivalentTo(m2))
}
This would then be used like this (everywhere)
expectMapsToBeEquivalent(g, clusterObjects.InfrastructureCluster.GetLabels(),
union(
map[string]string{
clusterv1.ClusterNameLabel: cluster.Name,
clusterv1.ClusterTopologyOwnedLabel: "",
},
ccInfrastructureClusterTemplateTemplateMetadata.Labels,
),
)
(It's not important/urgent, I can also take it over if you want, up to you)
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.
Look great, feel free to take it over :-)
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.
Opened a PR: #10851
What this PR does / why we need it:
Add union() to convert a potential nil map to an empty map, otherwise the test will faile due to compare a nil map and an empty one.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #10839
/area testing