fix: affinity label selector overriden by namespace selector #1609
Conversation
✅ Deploy Preview for vcluster-docs canceled.Built without sensitive environment variables
|
|
Will rebase on master, the base is currently 19 (should be fine, but will test rebased on master) |
rohantmp
force-pushed
the
fixAntiAffinitySelector
branch
from
f92c963
to
e9cc6ef
Compare
|
Hey @rohantmp! Can you explain how this solves the problem? |
|
Since we translate into a single namespace, the namespace selector was being translated into a labelselector based on the vcluster namespace labels, so it would override any translated labelselectors. Now this merges the translated label selector and the translated namespace selector into labelselectors |
|
verified the behavior locally too |
| isFocused := false | ||
| for _, testCase := range testCases { | ||
| if testCase.focus { | ||
| isFocused = true | ||
| break | ||
| } | ||
| } | ||
|
|
||
| for _, testCase := range testCases { | ||
| if isFocused && !testCase.focus { | ||
| continue | ||
| } |
There was a problem hiding this comment.
what is this for? selecting only a subset of tests to be run when developing?
There was a problem hiding this comment.
Yup, it fails at the end if any tests are focused.
There was a problem hiding this comment.
not sure I am a fan of this :D what's wrong with go test -v or something similar?
I think the issue here is that it's not using table driven test properly, because you'd have a proper breakdown of what tests failed with something like this
func Test(t *testing.T) {
testCases := []struct {
desc string
}{
{
desc: "",
},
}
for _, tC := range testCases {
t.Run(tC.desc, func(t *testing.T) {
})
}
}There was a problem hiding this comment.
So the use-case here was comparing the behaviors of each testcase with the new/old functionality alternatingly commented out. Also useful for using the debugger to step through specific tests. The current behavior does already does make it clear which case failed. While I agree t.Run is more canonical, I don't think it replaces this functionality. I mean now that I'm done fiddling with it, this code doesn't do anything till the next person has to fiddle with the tests. Also, sometimes you can't get to the test case you're trying to game out until you make the previous ones pass. It's not critical, but it makes the tests much more useful to me :)
There was a problem hiding this comment.
what do you mean? Also, sometimes you can't get to the test case you're trying to game out until you make the previous ones pass
You can absolutely do that with t.Run
this code doesn't do anything till the next person has to fiddle with the tests so it's essentially dead code no? And you can also use go test -v -run Test/subtest (with $ if you have subtest, subtest_something, etc so it only runs a complete match)
There was a problem hiding this comment.
Ahh, right. Didn't know you could use -run Test/subtest removing :)
| @@ -150,13 +173,17 @@ func TestPodAffinityTermsTranslation(t *testing.T) { | |||
|
|
|||
| assert.Assert(t, cmp.DeepEqual(events, testCase.expectedEvents), "Unexpected Event in the '%s' test case", testCase.name) | |||
| } | |||
|
|
|||
| assert.Assert(t, !isFocused, "test should not be focused, but if you've hit this, focused tests pass!") | |||
rohantmp
force-pushed
the
fixAntiAffinitySelector
branch
from
e9cc6ef
to
288e59a
Compare
Signed-off-by: Rohan CJ <rohantmp@gmail.com>
rohantmp
force-pushed
the
fixAntiAffinitySelector
branch
from
288e59a
to
4104a9c
Compare
What issue type does this pull request address? (keep at least one, remove the others)
/kind bugfix
What does this pull request do? Which issues does it resolve? (use
resolves #<issue_number>if possible)part-of ENG-2759
fixes #1484
Please provide a short message that should be published in the vcluster release notes
Fixed an issue where vcluster mistranslated affinity terms.
What else do we need to know?