Skip to content
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-0061 Allow custom task to be embedded in pipeline, design. #415

Merged
merged 1 commit into from
May 7, 2021

Conversation

ScrapCodes
Copy link
Contributor

@ScrapCodes ScrapCodes commented Apr 28, 2021

Thank you, @Tomcli, @pritidesai, @sbwsg, @imjasonh, @bobcatfish, @afrittoli and @vdemeester for taking a look !

@tekton-robot tekton-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Apr 28, 2021
@tekton-robot tekton-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 29, 2021
@ScrapCodes ScrapCodes force-pushed the tep-61-design branch 4 times, most recently from e99ee25 to f1718a9 Compare April 29, 2021 10:26
@Tomcli
Copy link
Contributor

Tomcli commented Apr 29, 2021

lgtm

2. *e2e tests* Add an e2e test for testing a pipelineRun with custom task embedded.

3. *documentation* Update documentation on custom tasks, to include embedded spec. Also add the
guide on validation for downstream custom task developers.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would recommend to have it split in two PRs rather than three. (1) Add documentation with a preview mode (2) Code changes + testing

At the same time, it's great to have it but does not have to be part of the TEP.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sounds good to me !

@@ -80,6 +80,7 @@ same namespace create resource. Related issue

1. Custom task controllers are to be developed by other parties. Custom task
specification validation by Tektoncd/Pipeline webhooks.
2. Handling `metadata` for embedded custom tasks i.e. `taskSpec`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not understand this non-goal here. Please explain a little more.

Also, we already have support for metadata while embedding the task specifications which must be propagated to Run or TaskRun:

type EmbeddedTask struct {
	// +optional
	Metadata PipelineTaskMetadata `json:"metadata,omitempty"`

	// TaskSpec is a specification of a task
	TaskSpec `json:",inline,omitempty"`
}

Copy link
Contributor Author

@ScrapCodes ScrapCodes Apr 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the question, @pritidesai. I am sorry, metadata is supported but it is limited to the sub-fields Labels and annotations

type PipelineTaskMetadata struct {
	// +optional
	Labels map[string]string `json:"labels,omitempty"`

	// +optional
	Annotations map[string]string `json:"annotations,omitempty"`
}

In the normal creation of a custom task resource object, e.g.:

apiVersion: custom.tekton.dev/v1alpha1
kind: TaskLoop
metadata:  # A metadata field e.g. name is supported 
  name: simpletaskloop
spec:
  taskRef:
    name: simpletask
  iterateParam: word
  timeout: 60s
  retries: 2

Once we embed a custom task, specifying it's name becomes irrelevant. But, still is it worth noting that we do not support all the metadata subfields?

I can update the non goal with above clarification, or completely remove it if it leads to confusion by stating the obvious.

@ScrapCodes ScrapCodes force-pushed the tep-61-design branch 2 times, most recently from 9346bc5 to 2a08f66 Compare May 3, 2021 12:02
@tekton-robot tekton-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 3, 2021
@bobcatfish bobcatfish added the kind/tep Categorizes issue or PR as related to a TEP (or needs a TEP). label May 3, 2021
@bobcatfish
Copy link
Contributor

/assign @pritidesai
/assign @sbwsg

@tekton-robot tekton-robot assigned pritidesai and ghost May 3, 2021
@ScrapCodes ScrapCodes requested a review from pritidesai May 4, 2021 10:41
@ScrapCodes
Copy link
Contributor Author

ScrapCodes commented May 4, 2021

Hi @pritidesai and @sbwsg , I have done a self pass and removed those items that seemed like stating the obvious. Please take a look.

There is an opened PR with implementation, tektoncd/pipeline#3901 .

Thank you, for helping this work move forward.

Next, whether it is a `Ref` or a `Spec`, validation logic will ensure, they have
non-empty values for, `APIVersion` and `Kind`.

Lastly document advice for downstream custom controllers to implement their
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great addition, thank you.

@ghost
Copy link

ghost commented May 4, 2021

/approve

@tekton-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbwsg

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 4, 2021
@pritidesai
Copy link
Member

thanks @ScrapCodes @Tomcli for the patience, sorry missed to review it. Looks great and exciting to have this feature.

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label May 7, 2021
@tekton-robot tekton-robot merged commit 76d9843 into tektoncd:main May 7, 2021
@ScrapCodes
Copy link
Contributor Author

Thanks a lot, @pritidesai, @sbwsg and @Tomcli.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/tep Categorizes issue or PR as related to a TEP (or needs a TEP). lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
Status: Implemented
Development

Successfully merging this pull request may close these issues.

5 participants