Add ClusterTask new CRD #312

nader-ziada · 2018-12-05T14:57:16Z

Fixes: #251

Proposed changes:

new type for ClusterTask
Add kind to TaskRef to differentiate between Task kinds
TaskInterface to be used by controllers, implemented by Task and ClusterTask
update taskRun and pipelineRun to use TaskInterface
update docs and tests

nader-ziada · 2018-12-05T14:57:50Z

/cc @imjasonh @shashwathi @bobcatfish

vdemeester

Small comments, otherwise looks good ! 👼

vdemeester · 2018-12-06T08:26:06Z

docs/Concepts.md

@@ -126,6 +126,10 @@ pull in your environment, so we provide a way for you to configure that by edit
 the `image`'s value in a configmap named
 [`config-entrypoint`](./../config/config-entrypoint.yaml).

+### ClusterTask
+
+Similar to `Tasks` but but with a cluster scope.


one but to remove 😅

vdemeester · 2018-12-06T08:28:10Z

pkg/apis/pipeline/v1alpha1/task_types.go

-// TaskStatus does not contain anything because Tasks on their own
-// do not have a status, they just hold data which is later used by a
-// TaskRun.
-type TaskStatus struct {


vdemeester · 2018-12-06T08:30:52Z

pkg/reconciler/v1alpha1/pipelinerun/resources/pipelinerunresolution.go

@@ -144,9 +150,10 @@ func ResolvePipelineRun(getTask resources.GetTask, getResource resources.GetReso

 		// Get all the resources that this task will be using, if any
 		inputs, outputs := getPipelineRunTaskResources(pt.Name, pr)
-		rtr, err := resources.ResolveTaskResources(&t.Spec, t.Name, inputs, outputs, getResource)
+		spec := t.TaskSpec()
+		rtr, err := resources.ResolveTaskResources(&spec, t.TaskMetadata().Name, inputs, outputs, getResource)


nit: Could we have a t.Name() function here ? (kind-of a shortcut so, not that important)

- issue 251: tektoncd#251 - new type for ClusterTask - Add kind to TaskRef to differentiate between Task kinds - TaskInterface to be used by controllers, implemeted by Task and ClusterTask - update taskRun and pipelineRun to use TaskInterface - update docs Signed-off-by: Nader Ziada <[email protected]>

imjasonh

Two small questions otherwise LGTM

imjasonh · 2018-12-06T15:21:00Z

config/300-clustertask.yaml

+metadata:
+  creationTimestamp: null
+  labels:
+    controller-tools.k8s.io: "1.0"


What does this do?

I just copied that from task.yaml but its left over from when these files were generated and should be removed. will take it out from both

imjasonh · 2018-12-06T15:22:16Z

pkg/apis/pipeline/v1alpha1/cluster_task_types.go

+	metav1.TypeMeta `json:",inline"`
+	// +optional
+	metav1.ListMeta `json:"metadata,omitempty"`
+	Items           []Task `json:"items"`


[]ClusterTask ?

yes it should be []ClusterTask, will fix that

- added copyright notice to yaml files in config/ - removed labels left over from kubebuilder generation from yaml files - fixed ClusterTask type in list items - add correct optional type in taskRun type Signed-off-by: Nader Ziada <[email protected]>

knative-metrics-robot · 2018-12-06T15:44:01Z

The following is the coverage report on pkg/.
Say /test pull-knative-build-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
cmd/controller/main.go	3.6%	3.5%	-0.1
pkg/apis/pipeline/v1alpha1/cluster_task_types.go	Do not exist	0.0%
pkg/apis/pipeline/v1alpha1/cluster_task_validation.go	Do not exist	0.0%
pkg/apis/pipeline/v1alpha1/task_types.go	100.0%	25.0%	-75.0
pkg/apis/pipeline/v1alpha1/taskrun_defaults.go	Do not exist	0.0%

vdemeester

/lgtm

knative-prow-robot · 2018-12-06T15:46:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pivotal-nader-ziada, vdemeester

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:

~~OWNERS~~ [pivotal-nader-ziada]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

tejal29 · 2018-12-06T20:04:27Z

docs/Concepts.md

@@ -126,6 +126,11 @@ pull in your environment, so we provide a way for you to configure that by edit
 the `image`'s value in a configmap named
 [`config-entrypoint`](./../config/config-entrypoint.yaml).

+### Cluster Task
+
+Similar to `Task` but with a cluster-wide scope. Cluster Tasks are available in 


Can we provide some examples of Cluster tasks to users? Like what kind of tasks could be cluster task Vs a normal Task.
Why shdn't all tasks be cluster tasks?

@tejal29 any task can be a ClusterTask, it depends on how operators set up their clusters, if they want their users to just use standard tasks available for everybody in all namespaces, or want users to setup their own. It is similar to ClusterBuildTemplates and BuildTemplates in Build

one thing that i noticed in the most recent user study is that i think we've started mixing some of our using docs into the concepts docs - I'd like to move some of the stuff we've got in concepts.md out and keep this doc pretty short and sweet, just about the high level concepts

something we can iterate on tho! and i think it might make sense to write up some guidelines on which docs go where 🤔

knative-prow-robot requested review from adrcunha and tejal29 December 5, 2018 14:57

knative-prow-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Dec 5, 2018

knative-prow-robot requested review from bobcatfish, imjasonh and shashwathi December 5, 2018 14:57

vdemeester reviewed Dec 6, 2018

View reviewed changes

nader-ziada force-pushed the clusterscope branch from 334ac69 to 4994de5 Compare December 6, 2018 14:51

imjasonh reviewed Dec 6, 2018

View reviewed changes

vdemeester approved these changes Dec 6, 2018

View reviewed changes

knative-prow-robot assigned vdemeester Dec 6, 2018

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 6, 2018

knative-prow-robot merged commit 2c9cf4f into tektoncd:master Dec 6, 2018

tejal29 reviewed Dec 6, 2018

View reviewed changes

nader-ziada deleted the clusterscope branch February 13, 2019 14:02

mchmarny unassigned vdemeester Mar 7, 2019

This was referenced Jan 9, 2023

[WIP] Picking out task_interface and pipeline_interface #5972

Closed

Cleanup resources of ClusterTask #5977

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ClusterTask new CRD #312

Add ClusterTask new CRD #312

nader-ziada commented Dec 5, 2018

nader-ziada commented Dec 5, 2018

vdemeester left a comment

vdemeester Dec 6, 2018

vdemeester Dec 6, 2018

vdemeester Dec 6, 2018

imjasonh left a comment

imjasonh Dec 6, 2018

nader-ziada Dec 6, 2018

imjasonh Dec 6, 2018

nader-ziada Dec 6, 2018

knative-metrics-robot commented Dec 6, 2018

vdemeester left a comment

knative-prow-robot commented Dec 6, 2018

tejal29 Dec 6, 2018

nader-ziada Dec 7, 2018

bobcatfish Dec 7, 2018

Add ClusterTask new CRD #312

Add ClusterTask new CRD #312

Conversation

nader-ziada commented Dec 5, 2018

Proposed changes:

nader-ziada commented Dec 5, 2018

vdemeester left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imjasonh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knative-metrics-robot commented Dec 6, 2018

vdemeester left a comment

Choose a reason for hiding this comment

knative-prow-robot commented Dec 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment