adding finally type (without implementation) at the pipeline level #2650
Conversation
tekton-robot
added
the
do-not-merge/work-in-progress
tekton-robot
added
the
size/XXL
|
This PR cannot be merged: expecting exactly one kind/ label Available
|
2 similar comments
|
This PR cannot be merged: expecting exactly one kind/ label Available
|
|
This PR cannot be merged: expecting exactly one kind/ label Available
|
|
The following is the coverage report on the affected files.
|
|
/kind feature |
tekton-robot
added
the
kind/feature
pritidesai
changed the title
There was a problem hiding this comment.
We shouldn't add anything anymore to v1alpha1. And any field that can't be convert (from v1beta1 to v1alpha1) should generate a ConversionError (I think that's how we named those). Now that we store v1beta1 and we have conversion webhook, we can make v1alpha1 API read-only (as no change of fields on it anymore).
| // Finally declares the list of Tasks that execute just before leaving the Pipeline | ||
| // i.e. either after all Tasks are finished executing successfully | ||
| // or after a failure which would result in ending the Pipeline | ||
| Finally []PipelineTask `json:"finally,omitempty"` |
There was a problem hiding this comment.
We shouldn't add anything on v1alpha1 now
There was a problem hiding this comment.
hey @vdemeester I have deleted finally from v1alpha1, have also added a unit test validating convertFrom, it should produce ConversionError while converting v1beta1 Pipeline with finally to v1alpha1, let me know if the whole conversion related changes looks fine
tekton-robot
added
size/XL
|
The following is the coverage report on the affected files.
|
There was a problem hiding this comment.
Thanks for submitting this incremental PR!! This really makes it a lot easier to review thoroughly and really focus on this instead of trying to review everything at once
- Could you fill in the release notes? We want to be ready to release at any time, which means that for a change like this, we could potentially release the types before the functionality, which IMO is totally worth it, but in that case it's especially important that the release notes explain what is being added and the implications
- Related to the last request: can this PR also include an incremental update to the docs, e.g. adding the new field to the docs and explaining that currently this does nothing?
|
The following is the coverage report on the affected files.
|
tekton-robot
added
size/XXL
|
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.
|
|
The following is the coverage report on the affected files.
|
|
Added summary of |
pritidesai
changed the title
tekton-robot
removed
the
do-not-merge/work-in-progress
|
The following is the coverage report on the affected files.
|
|
I have dropped |
|
The following is the coverage report on the affected files.
|
|
/test pull-tekton-pipeline-integration-tests |
| @@ -21,6 +21,7 @@ weight: 3 | |||
| - [Configuring execution results at the `Pipeline` level](#configuring-execution-results-at-the-pipeline-level) | |||
| - [Configuring the `Task` execution order](#configuring-the-task-execution-order) | |||
| - [Adding a description](#adding-a-description) | |||
| - [Adding `Finally` to the `Pipeline` (Preview)](#adding-finally-to-the-pipeline-preview) | |||
There was a problem hiding this comment.
ooo i like calling it a "preview" good call :D
| @@ -26,6 +26,8 @@ const ( | |||
| // resources when they cannot be converted to warn of a forthcoming | |||
| // breakage. | |||
| ConditionTypeConvertible apis.ConditionType = v1beta1.ConditionTypeConvertible | |||
| // Conversion Error message for a field not available in v1alpha1 | |||
| ConversionErrorFieldNotAvailableMsg = "the specified field/section is not available in v1alpha1" | |||
| Tasks: []PipelineTask{{Name: "mytask", TaskRef: &TaskRef{Name: "task"}}}, | ||
| }, | ||
| } | ||
| for _, version := range versions { |
There was a problem hiding this comment.
personally id still prefer to avoid the loop when we don't need it (YAGNI!) but if that's the only piece of feedback I have then lets just go with it as is :D
| source := ver | ||
| source.(*v1beta1.Pipeline).Spec.Finally = []v1beta1.PipelineTask{{Name: "finaltask", TaskRef: &TaskRef{Name: "task"}}} | ||
| got := &Pipeline{} | ||
| if err := got.ConvertFrom(context.Background(), source); err != nil { |
There was a problem hiding this comment.
im still a bit confused but ill go with it! i just dont understand why we go back to v1alpha1 (via ConvertFrom) at all - maybe that's after the controller is done reconciling, before we store the state back into etcd?
| return nil | ||
| } | ||
|
|
||
| // validatePipelineTasks ensures that pipeline tasks has unique label, pipeline tasks has specified one of | ||
| // taskRef or taskSpec, and in case of a pipeline task with taskRef, it has a reference to a valid task (task name) | ||
| func validatePipelineTasks(ctx context.Context, tasks []PipelineTask) *apis.FieldError { | ||
| func validatePipelineTasks(ctx context.Context, tasks []PipelineTask, finalTasks []PipelineTask) *apis.FieldError { |
There was a problem hiding this comment.
I made an example to show what im trying to say: https://play.golang.org/p/C2USaY-jimW (which was probably overkill, i think im just excited to write some code XD)
basically im wondering if you can keep the function as-is:
func validatePipelineTasks(ctx context.Context, tasks []PipelineTask)And then instead of calling it like this:
validatePipelineTasks(ctx, ps.Tasks, ps.Finally)You can do this:
And then instead of calling it like this:
validatePipelineTasks(ctx, append(ps.Tasks, ps.Finally...))Now I realize we'd lose the field error tho 🤔 i feel like it might be worth it! maybe instead of passing "spec.tasks" or "spec.finally" in for the field name we could use something generic like "task.name"
| @@ -270,6 +297,16 @@ func validatePipelineWorkspaces(wss []WorkspacePipelineDeclaration, pts []Pipeli | |||
| } | |||
| } | |||
| } | |||
| for tIdx, t := range finalTasks { | |||
| for wsIdx, ws := range t.Workspaces { | |||
There was a problem hiding this comment.
this is SO MINOR but in go i think we tend to prefer shorter variable names, so something like i and j in this case imo would be easier to read in go than tIdx and wsIdx
There was a problem hiding this comment.
Done, yup I hear you and with you, was following what we had for PipelineTask 😆 , updated PipelineTask code as well 👍
| @@ -245,7 +272,7 @@ func validatePipelineTaskName(ctx context.Context, prefix string, i int, t Pipel | |||
|
|
|||
| // validatePipelineWorkspaces validates the specified workspaces, ensuring having unique name without any empty string, | |||
| // and validates that all the referenced workspaces (by pipeline tasks) are specified in the pipeline | |||
| func validatePipelineWorkspaces(wss []WorkspacePipelineDeclaration, pts []PipelineTask) *apis.FieldError { | |||
| func validatePipelineWorkspaces(wss []WorkspacePipelineDeclaration, pts []PipelineTask, finalTasks []PipelineTask) *apis.FieldError { | |||
There was a problem hiding this comment.
this has the same deal as validatePipelineTasks - since finalTasks ARE PipelineTasks, i think you could pass them into this function as-is and not have to change it at all - the error message would have to be less generic but i totally think its worth it!
There was a problem hiding this comment.
its about the error message, its very easy to get lost in syntax errors with YAML and if we loose this field name, might make it even more harder 🤔 😞 I am anyways creating validation routines on PipelineTask as a function receiver like we talked about, will evaluate this along.
| }} | ||
| for _, tt := range tests { |
There was a problem hiding this comment.
oh, hah! good call, this didnt need a loop at all :D
| if err == nil { | ||
| t.Error("Pipeline.validateDeclaredResources() did not return error, wanted error") | ||
| t.Errorf("Pipeline.validateDeclaredResources() did not return error for invalid resource declarations: %s", tt.name) |
There was a problem hiding this comment.
thanks for making the error messages more descriptive!!
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bobcatfish 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
the
approved
|
The following is the coverage report on the affected files.
|
these changes are adding finally type and its validation, does not implement this new functionality.
|
The following is the coverage report on the affected files.
|
|
/hold Putting this hold just to make sure we wait for 0.13 before getting this merged 👼 |
tekton-robot
added
the
do-not-merge/hold
| Tasks: []PipelineTask{{Name: "mytask", TaskRef: &TaskRef{Name: "task"}}}, | ||
| }, | ||
| } | ||
| for _, version := range versions { |
There was a problem hiding this comment.
I would prefer to skip the loop to for now.
In L220 we cast to (*v1beta1.Pipeline), which would not work with other versions.
I think we would need to introduce a WithFinally interface to have this generic loop.
| func TestPipelineConversion_Failure(t *testing.T) { | ||
| versions := []apis.Convertible{&v1beta1.Pipeline{}} | ||
|
|
||
| tests := []struct { |
There was a problem hiding this comment.
NIT: this is just a single test so maybe we don't need the loop
| source := ver | ||
| source.(*v1beta1.Pipeline).Spec.Finally = []v1beta1.PipelineTask{{Name: "finaltask", TaskRef: &TaskRef{Name: "task"}}} | ||
| got := &Pipeline{} | ||
| if err := got.ConvertFrom(context.Background(), source); err != nil { |
There was a problem hiding this comment.
See my question above to @vdemeester and his reply.
As long as we serve v1alpha1 too, tools may request it. Since our pipelines are stored in v1beta1, to serve a v1alpha1 request we need to covert back to v1alpha1... which I feel it's like a slightly risky thing to do, but probably still better than not serving v1alpha1 anymore at all.
| @@ -260,12 +287,22 @@ func validatePipelineWorkspaces(wss []PipelineWorkspaceDeclaration, pts []Pipeli | |||
|
|
|||
| // Any workspaces used in PipelineTasks should have their name declared in the Pipeline's | |||
| // Workspaces list. | |||
| for ptIdx, pt := range pts { | |||
| for wsIdx, ws := range pt.Workspaces { | |||
| for i, pt := range pts { | |||
There was a problem hiding this comment.
NIT: I liked the original names ptIdx and wsIdx :)
|
|
||
| func ValidateTasksAndFinallySection(ps *PipelineSpec) *apis.FieldError { | ||
| if len(ps.Finally) != 0 && len(ps.Tasks) == 0 { | ||
| return apis.ErrInvalidValue(fmt.Sprintf("spec.tasks is empty but spec.finally has %d tasks", len(ps.Finally)), "spec.finally") |
There was a problem hiding this comment.
I added this to the pipeline validation test and it passes:
{
name: "valid metadata",
p: &Pipeline{
ObjectMeta: metav1.ObjectMeta{Name: "pipeline"},
Spec: PipelineSpec{
Description: "Empty pipeline",
},
},
}
The only thing that is not accepted today is PipelineSpec{} which returns:
--- FAIL: TestPipeline_Validate_Success (0.00s)
--- FAIL: TestPipeline_Validate_Success/valid_metadata#01 (0.00s)
pipeline_validation_test.go:110: Pipeline.Validate() returned error for valid Pipeline: valid metadata: expected at least one, got none: spec.description, spec.params, spec.resources, spec.tasks, spec.workspaces
FAIL
I don't see any point in having a pipeline without Tasks, so I feel like we should exclude that, but I think that extra validation would belong in a separated PR.
| p: &Pipeline{ | ||
| ObjectMeta: metav1.ObjectMeta{Name: "pipeline"}, | ||
| Spec: PipelineSpec{ | ||
| Tasks: nil, |
There was a problem hiding this comment.
NIT: we probably don't need Tasks: nil here, but it doesn't hurt
|
/hold cancel |
tekton-robot
removed
the
do-not-merge/hold
Changes
Updating pipeline type to support finally. These changes are adding a new type and validation, does not implement this new functionality.
Design doc: https://docs.google.com/document/d/1lxpYQHppiWOxsn4arqbwAFDo4T0-LCqpNa6p-TJdHrw/edit#
Part of #1684
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.
Release Notes