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

Tektoncd #264

Merged
merged 20 commits into from
Sep 9, 2019
Merged

Tektoncd #264

merged 20 commits into from
Sep 9, 2019

Conversation

kkasravi
Copy link
Contributor

@kkasravi kkasravi commented Aug 9, 2019

Which issue is resolved by this Pull Request:
Prerequiste to kubeflow/testing#424

Description of your changes:
Adds tektoncd crds and install to manifests - similar to argo.

The tektoncd installation has the crds split out into a separate kustomize target
since kustomize v2.0.3 doesn't do ordering. The tektoncd manifest adheres to best practices
and is similar to other kustomize targets where crds are separated.

Checklist:

  • Unit tests have been rebuilt:
    1. cd manifests/tests
    2. make generate
    3. make test

This change is Reviewable

@jlewi
Copy link
Contributor

jlewi commented Aug 15, 2019

@kkasravi I don't think there is more work to get tekton installed in our test clusters; (i.e. lets not close kubeflow/testing#424 after this PR is merged).

Test infra is here:
https://github.com/kubeflow/testing/tree/master/test-infra

So once we have a kustomize package for tekton we need to update that along with the instructions to actually get tekton deployed on our clusters.

Copy link
Contributor

@jlewi jlewi left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 36 files reviewed, 1 unresolved discussion (waiting on @jlewi, @kkasravi, and @richardsliu)


tektoncd/tektoncd-pipelines/base/kfctl_pr.yaml, line 2 at r2 (raw file):

---
apiVersion: tekton.dev/v1alpha1

Can I suggest moving any Kubeflow specific tasks/pipelines into a separate PR?
Maybe the first PR can just be a kustomize package for installing tekton?

@kkasravi
Copy link
Contributor Author

sure, i'll separate the others into a different PR

@krishnadurai
Copy link
Contributor

/cc @krishnadurai

@jlewi
Copy link
Contributor

jlewi commented Aug 18, 2019

@kkasravi Is this PR ready for review? If it is could you update the PR description please to capture what this PR is doing? e.g. I don't think this PR is installing Tekton on our test clusters. I think this PR is just defining a kustomize manifest for installing tekton and possibly some tekton tasks and pipelines for Kubeflow.

@kkasravi
Copy link
Contributor Author

kkasravi commented Sep 6, 2019

@kkasravi Is this PR ready for review? If it is could you update the PR description please to capture what this PR is doing? e.g. I don't think this PR is installing Tekton on our test clusters. I think this PR is just defining a kustomize manifest for installing tekton and possibly some tekton tasks and pipelines for Kubeflow.

yes this is ready to merge. I did a rebase from main and a make generate;make test
I also updated the PR description to make it clear this is just adding tekton crds, install manifests.
They haven't been included in the test environment or elsewhere.

@kkasravi
Copy link
Contributor Author

kkasravi commented Sep 6, 2019

/assign @jlewi

@jlewi
Copy link
Contributor

jlewi commented Sep 9, 2019

/lgtm
/approve
/hold

@kkasravi Could you update the PR description to explain why you went with creating manifests for the individual components? I believe customize has a custom resource that can be used to install all of the Tekton components.

I don't think its worth changing right now; if this PR works then lets go with it. It would be good to document though why we went with this approach for future reference. i.e. if the reason is simply that we didn't know about the CR then lets just capture that in the PR description so that we know that it might be worth investigating later.

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jlewi

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

@kkasravi
Copy link
Contributor Author

kkasravi commented Sep 9, 2019

/hold cancel

@k8s-ci-robot k8s-ci-robot merged commit 83f4041 into kubeflow:master Sep 9, 2019
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.

5 participants