feat(e2e): ensures namespace ignore label takes precedence #4073
Conversation
jaellio
force-pushed
the
e2eIgnoreNamespace
branch
3 times, most recently
from
85d4109 to
ec60fee
Compare
This test verifies that the ignore label on a namespace resource takes precedence over the values of the monitored-by label and the sidecar injection annotation if they have been applied to a Ns. The ignore label prevents sidecar injection from occuring in the namespace the label is applied to. Signed-off-by: jaellio <[email protected]>
jaellio
force-pushed
the
e2eIgnoreNamespace
branch
from
ec60fee to
5cd25cb
Compare
| var _ = OSMDescribe("Ignore Namespaces", | ||
| OSMDescribeInfo{ | ||
| Tier: 1, | ||
| Bucket: 2, |
There was a problem hiding this comment.
Will update to Tier: 2 after PR review is complete
There was a problem hiding this comment.
The Tier doesn't matter anymore. All tests run for PR workflows.
|
|
||
| // Add monitor-ignore to mesh with sidecar injection disabled | ||
| Expect(Td.AddNsToMesh(false, monitorIgnoreNs)).To(Succeed()) |
There was a problem hiding this comment.
This test case here seems unnecessary given that this test is meant to test the ignore label. Just testing ignore label with sidecar-injection enabled should suffice. This is a k8s functionality that we are testing now and not OSM, so I suggest simplifying this test to only test the ignore label and not all combinations of the matchExpression selector exposed by the k8s API.
There was a problem hiding this comment.
Could this check instead be expressed as a unit test? I don't see a test for this particular scenario, but similar tests are here:
osm/pkg/injector/webhook_test.go
Line 189 in 2cec5d7
@nojnhuh this test is primarily testing whether k8s matchExpression on the namespace selector configuration works as expected. I think the idea of this test was to confirm that we are correctly implementing the desired behavior (ignore label overrides sidecar-injection & monitor annotation and labels respectively). A simple e2e test as noted in #4073 (comment) should suffice in my opinion. I don't think we should be testing various permutations around k8s API behavior here. |
Signed-off-by: jaellio <[email protected]>
jaellio
force-pushed
the
e2eIgnoreNamespace
branch
from
ed2ace8 to
e49b267
Compare
Codecov Report
@@ Coverage Diff @@
## main #4073 +/- ##
==========================================
+ Coverage 68.41% 68.44% +0.03%
==========================================
Files 205 206 +1
Lines 11616 11673 +57
==========================================
+ Hits 7947 7990 +43
- Misses 3619 3633 +14
Partials 50 50
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
| var _ = OSMDescribe("Ignore Namespaces", | ||
| OSMDescribeInfo{ | ||
| Tier: 1, | ||
| Bucket: 2, |
There was a problem hiding this comment.
The Tier doesn't matter anymore. All tests run for PR workflows.
Description:
This test verifies that the ignore label on a namespace resource
takes precedence over the values of the monitored-by label and the
sidecar injection annotation if they have been applied to a Ns.
The ignore label prevents sidecar injection from occurring in the
namespace the label is applied to.
Testing done:
Affected area:
Please answer the following questions with yes/no.
Does this change contain code from or inspired by another project? No
Is this a breaking change? No