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

Add a controller which creates a tekton pipeline run from a prow job #11888

Merged
merged 7 commits into from
Apr 16, 2019

Conversation

ccojocar
Copy link
Member

@ccojocar ccojocar commented Mar 22, 2019

This controller is able to start a Tekton pipeline using the embedded PipelineRun specification defined in the prow job. The user will have to install beforehand all the resources referenced by this Tekton PipelineRun (such as Pipeline CRD, Task CRDs and PipelineResource CRDs).

Before starting the pipeline, the controller will create dynamically the git PipelineResoruce from prow job git reference and it will add it to the PipelineRun input parameters.

Design Doc

cc @rawlingsj @wbrefvem

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 22, 2019
@k8s-ci-robot
Copy link
Contributor

Hi @ccojocar. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. area/prow Issues or PRs related to prow area/prow/pod-utilities Issues or PRs related to prow's pod-utilities component sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Mar 22, 2019
@ccojocar
Copy link
Member Author

ccojocar commented Mar 22, 2019

/assign @fejta

@cjwagner
Copy link
Member

/assign
/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 22, 2019
@stevekuznetsov
Copy link
Contributor

Are there design documents for this? I would love to have a little more rigor for adding 11k LoC.

@spiffxp
Copy link
Member

spiffxp commented Mar 22, 2019

/hold
I agree, I'd really like to see some docs around this. I appreciate the contribution, but IMO dropping a massive PR with no initial conversation or documentation is not the path to a smooth landing in open source projects

eg:

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 22, 2019
@fejta
Copy link
Contributor

fejta commented Mar 23, 2019

/ok-to-test

@fejta fejta assigned fejta and cjwagner and unassigned cjwagner Mar 23, 2019
Copy link
Contributor

@fejta fejta left a comment

Choose a reason for hiding this comment

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

Agree that a document outlining what you are planning to do will be helpful.

It is not obvious to most people coming to this PR, for example, that this is largely copy/pasted from the build controller.

prow/cmd/pipeline/BUILD.bazel Outdated Show resolved Hide resolved
prow/cmd/pipeline/controller.go Outdated Show resolved Hide resolved
@ccojocar
Copy link
Member Author

My understanding was that @rawlingsj had some discussions with @cjwagner and @fejta before initialising this work. Anyhow, I will try to prepare a design document hopefully by the next sig-testing meeting that everyone is on the same page.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 24, 2019
@ccojocar
Copy link
Member Author

This is the design proposal which I would like to add it on today's sig-testing meeting agenda.

@ccojocar
Copy link
Member Author

In the design proposal review was decided to split this functionality in two separate controllers. The controller from this PR which starts directly the Tekton pipeline and another dedicated controller for Jenkins X, that will convert the Jenkins X pipeline file into Tektont CRDs and then start the appropriate pipeline. The Jenkins X controller, it will be implemented in a separate PR.

@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 28, 2019
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 31, 2019
@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Apr 12, 2019
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: e1dfcaf56889c73451367bce56517ca6e848bd82

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 15, 2019
@ccojocar
Copy link
Member Author

@fejta I addressed this issues. You can cancel the hold if the changes look fine to you. Thanks

// clusterToCtx converts the prow job's cluster to a cluster context
func clusterToCtx(cluster string) string {
if cluster == kube.DefaultClusterAlias {
return kube.InClusterContext
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems more appropriate for the other PR. I'd recommend taking out the commit from here as I don't think it works correctly. But happy to address it in a follow-up

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 15, 2019
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: a74ff7c390f71900c7e5389faa30c79a099515dd

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 16, 2019
Copy link
Contributor

@fejta fejta left a comment

Choose a reason for hiding this comment

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

/hold cancel

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Apr 16, 2019
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 3416bd2ce169915a9d7dbb550a33eff9740ab636

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ccojocar, fejta

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

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. area/prow/knative-build Issues or PRs related to prow's knative-build controller component area/prow/pod-utilities Issues or PRs related to prow's pod-utilities component area/prow Issues or PRs related to prow cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants