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
Make SA implicit in TestPropagatedParams #6185
Make SA implicit in TestPropagatedParams #6185
Conversation
/kind misc |
/test check-pr-has-kind-label |
@concaf: The specified target(s) for
The following commands are available to trigger optional jobs:
Use In response to this:
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. |
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.
Instead of adding a serviceAccountName to the test input (for tests that don't actually rely on the service account name at all), where it could be removed without breaking tests, I think it would be better to exclude the serviceaccountname from the want/got comparison for this set of tests.
fe775fb
to
d1447f7
Compare
@lbernick thanks for the quick response, it makes sense :) i've updated the PR |
That logs is not that usefull 🙃 |
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.
Because the "defaulting" mechanism will do its job, the expected TaskRun
or PipelineRun
will have a default serviceAccountName
set. We need to make sure we ignore this as well, using the same pattern as ignoreTypeMeta
and other cmpopts.IgnoreFields
.
d1447f7
to
5eafe20
Compare
test/propagated_params_test.go
Outdated
@@ -44,7 +44,8 @@ var ( | |||
ignoreConditions = cmpopts.IgnoreFields(duckv1.Status{}, "Conditions") | |||
ignoreContainerStates = cmpopts.IgnoreFields(corev1.ContainerState{}, "Terminated") | |||
ignoreStepState = cmpopts.IgnoreFields(v1beta1.StepState{}, "ImageID") | |||
ignoreTaskRunSpec = cmpopts.IgnoreFields(v1beta1.TaskRunSpec{}, "Resources") | |||
ignoreTaskRunSpec = cmpopts.IgnoreFields(v1beta1.TaskRunSpec{}, "Resources", "ServiceAccountName") |
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.
Is this variable used?
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.
do you mean ignoreTaskRunSpec
? it's used here -
pipeline/test/propagated_params_test.go
Line 126 in f0b8c16
ignoreTaskRunSpec, |
test/propagated_params_test.go
Outdated
@@ -44,7 +44,8 @@ var ( | |||
ignoreConditions = cmpopts.IgnoreFields(duckv1.Status{}, "Conditions") | |||
ignoreContainerStates = cmpopts.IgnoreFields(corev1.ContainerState{}, "Terminated") | |||
ignoreStepState = cmpopts.IgnoreFields(v1beta1.StepState{}, "ImageID") | |||
ignoreTaskRunSpec = cmpopts.IgnoreFields(v1beta1.TaskRunSpec{}, "Resources") | |||
ignoreTaskRunSpec = cmpopts.IgnoreFields(v1beta1.TaskRunSpec{}, "Resources", "ServiceAccountName") | |||
ignorePipelineRunSpec = cmpopts.IgnoreFields(v1beta1.PipelineRunSpec{}, "ServiceAccountName") |
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.
suggest renaming to "ignoreServiceAccountName" and adding a comment that we're ignoring it because the default can differ between platforms
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.
makes sense, done
Currently, `serviceAccountName: default` is added to some PipelineRuns in the test `TestPropagatedParams` and not the others. This commit removes it from all TaskRuns and PipelineRuns defined in the test. While this may seem inconsequential, making the service account implicit fixes running this test in OpenShift Pipelines where the default service account is not `default`. Further, the field `ServiceAccountName` has been configured to be ignored in both, `TaskRunSpec` and `PipelineRunSpec` as the actual output still contains the Service Account name.
5eafe20
to
cf9b51e
Compare
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lbernick 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 |
thanks @lbernick 😸 |
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
Changes
Submitter Checklist
As the author of this PR, please check off the items in this checklist:
functionality, content, code)
/kind <type>
. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tepRelease Notes