Change type of TaskSpec.Steps to a Step type that embeds Container #1060

imjasonh · 2019-07-10T00:28:31Z

Changes

Changes the type of the Steps field of the TaskSpec type to be a new type Step that embeds corev1.Container, rather than being a corev1.Container directly.

This opens up the option to add fields to our Step type that aren't present in corev1.Container (e.g., Script []string), and even to eventually not wrap the corev1.Container type at all, and instead define our own type that only includes the fields of corev1.Container we want the Tekton API to support.

YAML configs should not require any changes in response to this change, but users of generated API clients will need to change how they specify the Steps field to use the new type. My understanding of our backward-compatibility requirements means that this change doesn't qualify as "backward-incompatible" since YAMLs are not broken, but either way I think a.) this change and the options it opens up are worth the pain, and b.) it's better to make this kind of breaking change earlier rather than later, before too many more users come to depend on the generated API clients.

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

[y] Includes tests (if functionality changed/added)
[ n/a ] Includes docs (if user facing)
[y] Commit messages follow commit message best practices

See the contribution guide
for more details.

Release Notes

Changes the type of the Steps field to a type that embeds Container, instead of using the Container type directly.

cc @abayer

/hold
This change should go in after #1015 (@vdemeester) to avoid inflicting merge conflicts on that change.

imjasonh · 2019-07-10T13:51:20Z

/hold cancel

ghost · 2019-07-10T13:59:44Z

/lgtm

afrittoli

Thank you for this, I think it makes very much sense, it's a nice extension and cleanup.
I have a few minor questions, but nothing blocking really.

afrittoli · 2019-07-12T10:30:57Z

pkg/apis/pipeline/v1alpha1/image_resource.go


-// SetDestinationDirectory sets the destination for the resource


Just curious, did you drop the comments here because it's not actually implemented?

afrittoli · 2019-07-12T10:34:17Z

pkg/apis/pipeline/v1alpha1/pull_request_resource_test.go

-				Command:    []string{"/ko-app/pullrequest-init"},
-				Args:       []string{"-url", "https://example.com", "-path", "/workspace", "-mode", mode},
-				Env:        []corev1.EnvVar{},
+func testCases(mode string) []testcase {


Perhaps we should add a go fmt check to our CI, so we wouldn't need formatting fixes anymore - they make code review a bit harder when they mix with actual changes 👼

afrittoli · 2019-07-12T10:35:16Z

pkg/apis/pipeline/v1alpha1/task_types.go

@@ -63,6 +63,10 @@ type TaskSpec struct {
 	ContainerTemplate *corev1.Container `json:"containerTemplate,omitempty"`
 }

+type Step struct {


Some comments here would be nice, so the reason for this is not only in the commit message :)

My comment is still valid, but it's ok to add this in a follow-up.

afrittoli · 2019-07-12T10:41:57Z

pkg/apis/pipeline/v1alpha1/task_validation.go

@@ -237,3 +238,66 @@ func validateResourceType(r TaskResource, path string) *apis.FieldError {
 	}
 	return apis.ErrInvalidValue(string(r.Type), path)
 }
+


Uhm, I wonder why you dropped the merge module and moved CombineStepsWithStepTemplate in here.

afrittoli · 2019-07-12T10:45:21Z

pkg/apis/pipeline/v1alpha1/taskrun_validation_test.go

@@ -69,41 +68,35 @@ func TestTaskRun_Validate(t *testing.T) {
 }

 func TestTaskRunSpec_Invalidate(t *testing.T) {
-	tests := []struct {
+	for _, ts := range []struct {


Heh, dropping the extra tests var does not necessarily belong to this PR, but it's innocuous enough :)

afrittoli · 2019-07-12T10:54:58Z

pkg/reconciler/v1alpha1/taskrun/resources/input_resources.go

 	if err != nil {
 		return nil, nil, err
 	}

 	var storageVol []corev1.Volume
-	mountedSecrets := map[string]string{}
+	mountedSecrets := map[string]struct{}{}


Why is struct{}{} better than string{} here?

@afrittoli as we don't put anything in the value part of the map, using struct{}{} takes less memory space than string 😝

afrittoli · 2019-07-12T11:00:43Z

pkg/reconciler/v1alpha1/taskrun/resources/pod.go

-func makeWorkingDirInitializer(steps []corev1.Container) *corev1.Container {
-	workingDirs := make(map[string]bool)
+func makeWorkingDirInitializer(steps []v1alpha1.Step) *v1alpha1.Step {
+	workingDirs := map[string]struct{}{}


why is struct{}{} better than bool here?

Same as above, memory space

afrittoli · 2019-07-12T11:03:53Z

pkg/reconciler/v1alpha1/taskrun/taskrun_test.go

@@ -406,11 +409,6 @@ func TestReconcile(t *testing.T) {
 					tb.VolumeMount("downward", "/builder/downward"),
 					tb.VolumeMount("workspace", workspaceDir),
 					tb.VolumeMount("home", "/builder/home"),
-					tb.Resources(tb.Requests(


Why did you drop tb.Resources here (and below)?

afrittoli · 2019-07-12T11:05:47Z

test/builder/step.go

+)
+
+// StepOp is an operation which modifies a Container struct.
+type StepOp func(*v1alpha1.Step)


Nice, dedicated test builder module :) 👍

bobcatfish · 2019-07-15T14:22:18Z

YAML configs should not require any changes in response to this change, but users of generated API clients will need to change how they specify the Steps field to use the new type. My understanding of our backward-compatibility requirements means that this change doesn't qualify as "backward-incompatible" since YAMLs are not broken

That matches my understanding as well! I think if we eventually start publishing go libs officially (I think @vdemeester was suggesting this earlier today...) we might want to revisit this but at the moment it should be fine.

Also we mentioned this at last week's working group meeting and there was no major outcry (possible folks didn't realize the implications tho? + @skaegi @mnuttall for visibility if you are depending on the libs)

bobcatfish · 2019-08-12T18:28:57Z

Just pinging this - looks like it won't make it in for 0.6 probably but maybe we can do it for 0.7 @imjasonh ?

bobcatfish · 2019-08-13T15:53:10Z

tekton-robot · 2019-08-13T18:21:29Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: afrittoli, ImJasonH

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~~ [ImJasonH,afrittoli]

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

tekton-robot · 2019-08-13T18:23:48Z

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

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1alpha1/cluster_resource.go	71.8%	71.1%	-0.7
pkg/apis/pipeline/v1alpha1/merge.go	Do not exist	72.4%
pkg/apis/pipeline/v1alpha1/step_replacements.go	Do not exist	100.0%
pkg/apis/pipeline/v1alpha1/substitution.go	Do not exist	98.1%
pkg/reconciler/v1alpha1/pipelinerun/pipelinerun.go	74.6%	82.4%	7.8
pkg/reconciler/v1alpha1/taskrun/resources/input_resources.go	91.5%	91.7%	0.1
pkg/reconciler/v1alpha1/taskrun/resources/output_resource.go	92.0%	91.7%	-0.3
pkg/reconciler/v1alpha1/taskrun/resources/pod.go	88.5%	88.9%	0.4
test/builder/step.go	Do not exist	10.5%

tekton-robot · 2019-08-13T18:34:15Z

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

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1alpha1/cluster_resource.go	71.8%	71.1%	-0.7
pkg/apis/pipeline/v1alpha1/merge.go	Do not exist	72.4%
pkg/apis/pipeline/v1alpha1/step_replacements.go	Do not exist	100.0%
pkg/apis/pipeline/v1alpha1/substitution.go	Do not exist	98.1%
pkg/reconciler/v1alpha1/pipelinerun/pipelinerun.go	74.6%	82.4%	7.8
pkg/reconciler/v1alpha1/taskrun/resources/input_resources.go	91.5%	91.7%	0.1
pkg/reconciler/v1alpha1/taskrun/resources/output_resource.go	92.0%	91.7%	-0.3
pkg/reconciler/v1alpha1/taskrun/resources/pod.go	88.5%	88.9%	0.4
test/builder/step.go	Do not exist	10.5%

tekton-robot · 2019-08-13T18:38:08Z

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

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1alpha1/cluster_resource.go	71.8%	71.1%	-0.7
pkg/apis/pipeline/v1alpha1/merge.go	Do not exist	72.4%
pkg/apis/pipeline/v1alpha1/step_replacements.go	Do not exist	100.0%
pkg/apis/pipeline/v1alpha1/substitution.go	Do not exist	98.1%
pkg/reconciler/v1alpha1/pipelinerun/pipelinerun.go	74.6%	82.4%	7.8
pkg/reconciler/v1alpha1/taskrun/resources/input_resources.go	91.5%	91.7%	0.1
pkg/reconciler/v1alpha1/taskrun/resources/output_resource.go	92.0%	91.7%	-0.3
pkg/reconciler/v1alpha1/taskrun/resources/pod.go	88.5%	88.9%	0.4
test/builder/step.go	Do not exist	10.5%

- move pkg/merge into v1alpha1 to avoid circular dependency - move pkg/substitution into v1alpha1 to avoid circular dependency - change resource interface from Get*ContainerSpec -> *Steps - change artifact storage interface from GetCopy*Storage*ContainerSpec -> *Steps - removed some unnecessarily table-driven tests - squashed whitespace and brackets where possible

tekton-robot · 2019-08-13T20:19:48Z

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

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1alpha1/cluster_resource.go	71.8%	71.1%	-0.7
pkg/apis/pipeline/v1alpha1/merge.go	Do not exist	72.4%
pkg/apis/pipeline/v1alpha1/step_replacements.go	Do not exist	100.0%
pkg/apis/pipeline/v1alpha1/substitution.go	Do not exist	98.1%
pkg/reconciler/v1alpha1/pipelinerun/pipelinerun.go	74.6%	82.4%	7.8
pkg/reconciler/v1alpha1/taskrun/resources/input_resources.go	91.5%	91.7%	0.1
pkg/reconciler/v1alpha1/taskrun/resources/output_resource.go	92.0%	91.7%	-0.3
pkg/reconciler/v1alpha1/taskrun/resources/pod.go	88.5%	88.9%	0.4
test/builder/step.go	Do not exist	10.5%

afrittoli

Nice, thank you!

Perhaps as a follow-up you could add some comments about the reason for Step struct.

afrittoli · 2019-08-13T20:46:49Z

pkg/apis/pipeline/v1alpha1/merge.go

@@ -14,7 +14,7 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */

-package merge
+package v1alpha1


Does this make this module now part of our (not yet stable) api surface?

My understanding of the API surface guarantee was that (once we commit to it) users' YAML must continue to be accepted. I don't think library or generated client APIs are in scope. @bobcatfish please correct me if that's not the case!

afrittoli · 2019-08-13T20:54:37Z

pkg/apis/pipeline/v1alpha1/step_replacements.go

+ limitations under the License.
+*/
+
+package v1alpha1


It sounds like a good idea to have the same and only place where this is used.
In case in future we extend Steps via duck typing, it's nice to have this in the API :)

<soapbox>
I'd actually like to see if we can move some of these things out of this package, so that v1alpha1 only contains structs, and no validation, merging, substitution, etc.

It's useful to have a low-dependency structs-only package for integrators to use only to send requests to a Tekton-enabled cluster, without having to import all the code Tekton uses to deal with those requests.

Future change though. 😄
</soapbox>

afrittoli · 2019-08-13T21:04:19Z

pkg/apis/pipeline/v1alpha1/task_types.go

@@ -63,6 +63,10 @@ type TaskSpec struct {
 	ContainerTemplate *corev1.Container `json:"containerTemplate,omitempty"`
 }

+type Step struct {


My comment is still valid, but it's ok to add this in a follow-up.

afrittoli · 2019-08-13T21:12:47Z

pkg/reconciler/v1alpha1/taskrun/resources/pod.go

-						cn := v1alpha1.CreateDirContainer("default-image-output", fmt.Sprintf("%s/%s", v1alpha1.TaskOutputImageDefaultDir, r.Name))
-						cn.VolumeMounts = append(cn.VolumeMounts, implicitVolumeMounts...)
-						makeDirSteps = append(makeDirSteps, cn)
+						s := v1alpha1.CreateDirStep("default-image-output", fmt.Sprintf("%s/%s", v1alpha1.TaskOutputImageDefaultDir, r.Name))


oh, this is very thorough :D

afrittoli · 2019-08-13T22:02:55Z

I'm ready to LGTM on this patch, but holding off since it's not targeted for 0.6 as far as I can see - perhaps this could be merged once 0.6 is done

afrittoli · 2019-08-13T23:43:04Z

/lgtm

Step was added in tektoncd#1060

Step was added in #1060

tekton-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Jul 10, 2019

tekton-robot requested a review from vdemeester July 10, 2019 00:28

googlebot added the cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit label Jul 10, 2019

tekton-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jul 10, 2019

imjasonh force-pushed the steps branch 2 times, most recently from f34f993 to 0471664 Compare July 10, 2019 13:19

imjasonh changed the title ~~WIP: Change type of TaskSpec.Steps to a Step type that embeds Container~~ Change type of TaskSpec.Steps to a Step type that embeds Container Jul 10, 2019

tekton-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 10, 2019

tekton-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 10, 2019

tekton-robot assigned ghost Jul 10, 2019

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 10, 2019

imjasonh force-pushed the steps branch from 0471664 to a4af684 Compare July 10, 2019 14:09

tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Jul 10, 2019

afrittoli approved these changes Jul 12, 2019

View reviewed changes

imjasonh force-pushed the steps branch from a4af684 to b1c077a Compare August 13, 2019 18:21

imjasonh force-pushed the steps branch from b1c077a to 45d9059 Compare August 13, 2019 18:32

imjasonh force-pushed the steps branch from 45d9059 to 7092bf8 Compare August 13, 2019 18:36

imjasonh force-pushed the steps branch from 7092bf8 to 3a7389b Compare August 13, 2019 20:17

afrittoli reviewed Aug 13, 2019

View reviewed changes

tekton-robot assigned afrittoli Aug 13, 2019

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 13, 2019

tekton-robot merged commit 7a0454b into tektoncd:master Aug 13, 2019

imjasonh added a commit to imjasonh/pipeline that referenced this pull request Aug 13, 2019

Document rationale for the Step type

ed93291

Step was added in tektoncd#1060

imjasonh mentioned this pull request Aug 13, 2019

Document rationale for the Step type #1193

Merged

tekton-robot pushed a commit that referenced this pull request Aug 14, 2019

Document rationale for the Step type

360706a

Step was added in #1060

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change type of TaskSpec.Steps to a Step type that embeds Container #1060

Change type of TaskSpec.Steps to a Step type that embeds Container #1060

imjasonh commented Jul 10, 2019

imjasonh commented Jul 10, 2019

ghost commented Jul 10, 2019

afrittoli left a comment

afrittoli Jul 12, 2019

afrittoli Jul 12, 2019

afrittoli Jul 12, 2019

afrittoli Aug 13, 2019

afrittoli Jul 12, 2019

afrittoli Jul 12, 2019

afrittoli Jul 12, 2019

vdemeester Jul 31, 2019

afrittoli Jul 12, 2019

vdemeester Jul 31, 2019

afrittoli Jul 12, 2019

afrittoli Jul 12, 2019

bobcatfish commented Jul 15, 2019

bobcatfish commented Aug 12, 2019

bobcatfish commented Aug 13, 2019

tekton-robot commented Aug 13, 2019

tekton-robot commented Aug 13, 2019

tekton-robot commented Aug 13, 2019

tekton-robot commented Aug 13, 2019

tekton-robot commented Aug 13, 2019

afrittoli left a comment

afrittoli Aug 13, 2019

imjasonh Aug 13, 2019

afrittoli Aug 13, 2019

imjasonh Aug 13, 2019 •

edited

Loading

afrittoli Aug 13, 2019

afrittoli Aug 13, 2019

afrittoli commented Aug 13, 2019 •

edited

Loading

afrittoli commented Aug 13, 2019


		// SetDestinationDirectory sets the destination for the resource

Change type of TaskSpec.Steps to a Step type that embeds Container #1060

Change type of TaskSpec.Steps to a Step type that embeds Container #1060

Conversation

imjasonh commented Jul 10, 2019

Changes

Submitter Checklist

Release Notes

imjasonh commented Jul 10, 2019

ghost commented Jul 10, 2019

afrittoli 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobcatfish commented Jul 15, 2019

bobcatfish commented Aug 12, 2019

bobcatfish commented Aug 13, 2019

tekton-robot commented Aug 13, 2019

tekton-robot commented Aug 13, 2019

tekton-robot commented Aug 13, 2019

tekton-robot commented Aug 13, 2019

tekton-robot commented Aug 13, 2019

afrittoli 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 Aug 13, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afrittoli commented Aug 13, 2019 • edited Loading

afrittoli commented Aug 13, 2019

imjasonh Aug 13, 2019 •

edited

Loading

afrittoli commented Aug 13, 2019 •

edited

Loading