TestReconcile_ExplicitDefaultSA flakiness fix 🍂 #3189
Conversation
|
@vdemeester: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
tekton-robot
added
kind/misc
|
/release-note-none |
tekton-robot
added
release-note-none
|
The following is the coverage report on the affected files.
|
| Condition informersv1alpha1.ConditionInformer | ||
| Pod coreinformers.PodInformer | ||
| ConfigMap coreinformers.ConfigMapInformer | ||
| ServiceAccount coreinformers.ServiceAccountInformer |
There was a problem hiding this comment.
@vdemeester I am trying to understand this, why making this as part of informer? and creating fake service account informer?
There was a problem hiding this comment.
From https://github.com/tektoncd/pipeline/pull/3189/files#r486827794, this is an "helper" to remove the need to do the clients.Kube.CoreV1().ServiceAccounts(tc.taskRun.Namespace).Create during the test.
| t.Fatal(err) | ||
| } | ||
| } | ||
| c.Kube.PrependReactor("*", "serviceaccounts", AddToInformer(t, i.ServiceAccount.Informer().GetIndexer())) |
There was a problem hiding this comment.
This is similar to the other resources we have pipeline, pipelinerun, etc. and I am guessing this would be needed for all service accounts that a task/pipeline is expecting including default. i.e. update all unit tests to utilize this fake service account? how is it different than what we have today 🤔
clients.Kube.CoreV1().ServiceAccounts(tc.taskRun.Namespace).Create(&corev1.ServiceAccount{
There was a problem hiding this comment.
As of today, we don't have the reactor, so the informer are/might be out-of-sync. This can cause errors during tests where the serviceaccount is not found. Having this fixes that problem.
There was a problem hiding this comment.
but indeed, we should update the unit tests that manually create the ServiceAccounts to use this instead
|
sounds good, thanks @vdemeester lets go with this. Next we will be updating rest of the unit tests in |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: pritidesai 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 |
tekton-robot
added
approved
|
@vdemeester: PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
This tries to fix the test flakiness by making sure the informers for serviceaccounts are up-to-date when we execute the tests. Otherwise we may run into failure because the serviceaccount is not found. Signed-off-by: Vincent Demeester <[email protected]>
vdemeester
force-pushed
the
3160-sa-unit-failure
branch
from
ed256c5 to
99de1f5
Compare
tekton-robot
added
size/S
vdemeester
removed
the
needs-rebase
|
/lgtm |
|
Part of #2185 - might need to make same change for other tests as well? |
Changes
This tries to fix the test flakiness by making sure the informers for
serviceaccounts are up-to-date when we execute the tests. Otherwise we
may run into failure because the serviceaccount is not found.
Signed-off-by: Vincent Demeester [email protected]
/cc @pritidesai @bobcatfish
Submitter Checklist
These are the criteria that every PR should meet, please check them off as you
review them:
See the contribution guide for more details.
Double check this list of stuff that's easy to miss:
cmddir, please updatethe release Task to build and release this image.
Reviewer Notes
If API changes are included, additive changes must be approved by at least two OWNERS and backwards incompatible changes must be approved by more than 50% of the OWNERS, and they must first be added in a backwards compatible way.
/kind misc
/area testing
/release-note-none