Use the old topology key for e2e tests #814
Conversation
k8s-ci-robot
added
cncf-cla: yes
k8s-ci-robot
added
the
size/S
Pull Request Test Coverage Report for Build 1754
💛 - Coveralls |
|
Can we loop through and check both are present? That is the intention of our code, to provision PVs with both so that they are backwards compatible, we should test it. |
|
I don't mind merging to unblock CI but ideally we should test our driver-specific behavior in our driver-specific test suite otherwise what's the point of it! This kind of thing won't be covered by the external test suite. |
|
/lgtm to unblock CI |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ayberk, wongma7 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 |
| for _, v := range t.persistentVolume.Spec.NodeAffinity.Required.NodeSelectorTerms[0].MatchExpressions { | ||
| if v.Key == "topology.ebs.csi.aws.com/zone" { | ||
| keyFound = true | ||
| Expect(v.Key).To(Equal(t.storageClass.AllowedTopologies[0].MatchLabelExpressions[0].Key)) |
There was a problem hiding this comment.
This statement is pointless, it's a tautology, if you're in this if statement then this is already guaranteed to be true
There was a problem hiding this comment.
never mind, I confused storageClass allowedTopoloigies with the PV spec field? I'm a bit lost TBH,whether my SC has the old key or new key, the PV should have both old or new key right?
There was a problem hiding this comment.
Yeah PV has the both keys, but SC has one (in e2e tests it's always the oldest one, see above tests/e2e/driver/ebs_csi_driver.go). So we have to make sure we're comparing the correct keys.
For example, this test fails when t.persistentVolume.Spec.NodeAffinity.Required.NodeSelectorTerms[0].MatchExpressions[0].Key is not topology.ebs.csi.aws.com/zone, which is possible because we also have topology.kubernetes.io/zone in the slice.
Is this a bug fix or adding new feature?
/bug
What is this PR about? / Why do we need it?
e2e tests assumes only one topology key, so checks are hardcoded with the first element of each slice. Due to the recent addition of the well-known label as topology key, these checks might fail based on the ordering of the slice. This PR fixes it.
What testing is done?
Tested on my account and verified it works and does what I expect it to do.