-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
[TEP-0091] support remote v1 task verification #6764
Conversation
Skipping CI for Draft Pull Request. |
1b586a4
to
ea1adbd
Compare
@@ -1016,6 +1039,288 @@ func TestGetTaskFunc_VerifyError(t *testing.T) { | |||
} | |||
} | |||
|
|||
func TestGetTaskFunc_V1Task_VerifyNoError(t *testing.T) { |
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.
The tests in this PR are all copied from current v1beta1 tasks. I tried to merge them into 1 test but seems not so straightforward. Maybe the easiest way for now is to have separate tests? Any suggestions are welcome
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.
I'm guessing this tests might change a bit once we swap the storage version to v1 - I think it's worth checking them to see if we can merge the tests or at least come up with a few helper methods to reduce the amount of copied code between tests
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.
Yes, I will pair with @JeromeJu on fixing the tests for the swap PR.
I tried to merge the v1 tests into v1beta1 tests but it still requires lots of test code changes. But helper functions may help.
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.
This could be tackled as part of #5820
This PR only has 2 lines of code, other code changes are copied tests and helper functions. |
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
/assign |
}, | ||
}, | ||
} | ||
tr := parse.MustParseV1beta1TaskRun(t, fmt.Sprintf(` |
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 there a reason we are using a v1beta1TaskRun and not a v1TaskRun?
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.
yes, for now in
Lines 68 to 71 in 4924b51
type Data struct { | |
PipelineRuns []*v1beta1.PipelineRun | |
Pipelines []*v1beta1.Pipeline | |
TaskRuns []*v1beta1.TaskRun |
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.
currently in #6444 we are swapping to v1, so maybe we should keep both in either case?
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.
I think this is fine-- a v1beta1 TaskRun can reference a v1 Task, and it looks like this is correctly testing that case and will be swapped to a v1 TaskRun in Jerome's PR. @Yongxuanzhang can you update the existing test class names to indicate that they're resolving a v1beta1 Task? @JeromeJu just make sure your PR doesn't change the test cases referencing a v1beta1 Task, otherwise they will become redundant with this test case.
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.
Thanks @lbernick , I left all v1beta1 verifications as they are in both unit tests and e2e tests, only modifying the apiVersion of the pr/tr referencing the v1beta1 pipeline/task.
ea1adbd
to
8c8bfd6
Compare
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
8c8bfd6
to
755d4dd
Compare
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
}, | ||
}, | ||
} | ||
tr := parse.MustParseV1beta1TaskRun(t, fmt.Sprintf(` |
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.
I think this is fine-- a v1beta1 TaskRun can reference a v1 Task, and it looks like this is correctly testing that case and will be swapped to a v1 TaskRun in Jerome's PR. @Yongxuanzhang can you update the existing test class names to indicate that they're resolving a v1beta1 Task? @JeromeJu just make sure your PR doesn't change the test cases referencing a v1beta1 Task, otherwise they will become redundant with this test case.
@@ -1016,6 +1039,288 @@ func TestGetTaskFunc_VerifyError(t *testing.T) { | |||
} | |||
} | |||
|
|||
func TestGetTaskFunc_V1Task_VerifyNoError(t *testing.T) { |
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.
This could be tackled as part of #5820
This commit adds the support for v1 task verification. Previously we only support v1beta1 verificaiton. Signed-off-by: Yongxuan Zhang yongxuanzhang@google.com
755d4dd
to
3da0f9e
Compare
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: JeromeJu, 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 |
/retest |
/lgtm |
Changes
This commit adds the support for v1 task verification. Previously we only support v1beta1 verification.
Part of #6729
/kind feature
Signed-off-by: Yongxuan Zhang yongxuanzhang@google.com
Submitter Checklist
As the author of this PR, please check off the items in this checklist:
/kind <type>
. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tepRelease Notes