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

feat(pipelineloop) : Support latest tektoncd pipeline version. #602

Merged

Conversation

ScrapCodes
Copy link
Contributor

@ScrapCodes ScrapCodes commented May 27, 2021

Which issue is resolved by this Pull Request:
Resolves #

Description of your changes:
Updated to latest tektoncd version.
go mod tidy
and passed all tests.
Environment tested:

  • Python Version (use python --version):
  • Tekton Version (use tkn version):
  • Kubernetes Version (use kubectl version):
  • OS (e.g. from /etc/os-release):

Checklist:

@ScrapCodes
Copy link
Contributor Author

tested with the included examples i.e. ls examples/*taskspec.yaml

@ScrapCodes
Copy link
Contributor Author

cc @Tomcli

@Tomcli
Copy link
Member

Tomcli commented May 28, 2021

thanks @ScrapCodes, can you update the readme saying that to use the taskSpec example, they will have to deploy master branch Tekton or upcoming Tekton v0.25? Thanks.

/cc @pugangxa @jinchihe we are updating the pipelineloop reconcile logic to handle the new custom task spec from tektoncd/pipeline#3901
Let us know if you see any issue in this PR. Thanks.

@pugangxa
Copy link
Contributor

Thanks @ScrapCodes @Tomcli , Great to saw the PR for tekton merged. Took a look at the PR and generally it's fine. Just some questions:

@ScrapCodes
Copy link
Contributor Author

When the parent pipelinerun is deleted, the embeded custom resource will be delete right?

Embedded custom resource is not created in etcd at any point in time.

@Tomcli
Copy link
Member

Tomcli commented May 29, 2021

Thanks @ScrapCodes @Tomcli , Great to saw the PR for tekton merged. Took a look at the PR and generally it's fine. Just some questions:

#512 is only for custom resources that need self reference. E.g. pipeline loops for recursion only

  • So later we want this embed method to be the standard method right? Then I think we need some DSL side changes to support it.

Yes, we will need to update the API and SDK to support this new syntax later on. Our goal is to have them based on the Tekton v0.25 release, but we will have some experimental code using the Tekton master branch as the meantime.

  • When the parent pipelinerun is deleted, the embeded custom resource will be delete right?

As @ScrapCodes mentioned, the new embedded custom task won't be a separate pipelineloop CR because the pipelineloop controller can get the pipelineloop spec from Tekton Run.

Here is the high level diagram, we are not introducing the validation webhook on the embedded spec yet.

image

@pugangxa
Copy link
Contributor

pugangxa commented May 29, 2021

Thanks @ScrapCodes @Tomcli , Great to saw the PR for tekton merged. Took a look at the PR and generally it's fine. Just some questions:

#512 is only for custom resources that need self reference. E.g. pipeline loops for recursion only

Yeah, it's reasonable to do it only for recursion.

  • So later we want this embed method to be the standard method right? Then I think we need some DSL side changes to support it.

Yes, we will need to update the API and SDK to support this new syntax later on. Our goal is to have them based on the Tekton v0.25 release, but we will have some experimental code using the Tekton master branch as the meantime.

Got it. So I think we can make it a standard way but still need generate new CR for recursion, but yes the logic can all be done in DSL which is more friendly to users.

  • When the parent pipelinerun is deleted, the embeded custom resource will be delete right?

As @ScrapCodes mentioned, the new embedded custom task won't be a separate pipelineloop CR because the pipelineloop controller can get the pipelineloop spec from Tekton Run.

Here is the high level diagram, we are not introducing the validation webhook on the embedded spec yet.

Thanks for the information, from the diagram user will get the response once the pipelinerun created successfully no matter the Run CR creation status? I think we need enhance it later.

@Tomcli
Copy link
Member

Tomcli commented Jun 1, 2021

Thanks for the information, from the diagram user will get the response once the pipelinerun created successfully no matter the Run CR creation status? I think we need enhance it later.

@pugangxa Yes right now the Run CR just created without any validation. From the TEP 61, the recommended feedback is to have some kind of async validation from the custom controller. So the not all the RUN CRs will be pending when one of the custom task validation webhooks is failing. Let me open another issue to track these enhancement.

To enhance the pipelineloop with embedded spec, we need

  1. async validation from the custom controller
  2. have custom task timeout when the custom controller is not available. Implement timeout for custom tasks. tektoncd/pipeline#3976

@Tomcli
Copy link
Member

Tomcli commented Jun 1, 2021

/lgtm

@Tomcli
Copy link
Member

Tomcli commented Jun 1, 2021

@pugangxa any last minute comment before I merge it?

@pugangxa
Copy link
Contributor

pugangxa commented Jun 2, 2021

@pugangxa any last minute comment before I merge it?

Thanks Tommy, no more comments from my side.

@Tomcli
Copy link
Member

Tomcli commented Jun 2, 2021

/approve

@google-oss-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ScrapCodes, Tomcli

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

@google-oss-robot google-oss-robot merged commit 052726a into kubeflow:master Jun 2, 2021
@ScrapCodes ScrapCodes deleted the pipelineloop_taskspec_support branch June 3, 2021 09:18
@ScrapCodes ScrapCodes restored the pipelineloop_taskspec_support branch September 3, 2021 09:09
@ScrapCodes ScrapCodes deleted the pipelineloop_taskspec_support branch September 3, 2021 09:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants