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 cluster and cluster bindings #60

Merged
merged 6 commits into from
Sep 24, 2018
Merged

Conversation

shashwathi
Copy link
Contributor

Fixes #21

Addresses the following requirements in issue

  • pipelineparams should have clusters
  • pipelines should have clusterBindings
  • tasks should have cluster inputs

/assign @bobcatfish
/assign @pivotal-nader-ziada

@knative-prow-robot knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 21, 2018
@knative-prow-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: shashwathi
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: bobcatfish

If they are not already assigned, you can assign the PR to them by writing /assign @bobcatfish in a comment when ready.

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

@nader-ziada
Copy link
Member

nader-ziada commented Sep 21, 2018

@shashwathi it might conflict with the optional types PR #56 depending on what gets merged in first, but in general take a look at the api conventions to add the +optional comment

@shashwathi
Copy link
Contributor Author

@pivotal-nader-ziada : I will update this PR to include optional comment for fields I have added.

/hold
Hold this PR until #56 is merged

@knative-prow-robot knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 21, 2018
@shashwathi
Copy link
Contributor Author

/test pull-knative-build-pipeline-unit-tests

@shashwathi
Copy link
Contributor Author

/hold cancel

Updated with optional tags

@knative-prow-robot knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 21, 2018
@nader-ziada
Copy link
Member

/lgtm

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 21, 2018
Copy link
Contributor

@tejal29 tejal29 left a comment

Choose a reason for hiding this comment

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

few nits and go coding conventions

properties:
clusterName:
type: string
inputName:
Copy link
Contributor

Choose a reason for hiding this comment

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

IIUC, inputName stands for what input this cluster is bound to? Can you rename it to boundTo or something?

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 is auto generated code. If we change the definition, crds should change as well.

// ClusterName is the string that the PipelineParams will use to identify this source.
ClusterName string `json:"clusterName"`
// InputName is the string the Task will use to identify this cluster in its inputs.
InputName string `json:"inputName"`
Copy link
Contributor

@tejal29 tejal29 Sep 21, 2018

Choose a reason for hiding this comment

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

Lets get ClusterBinding and SourceBinding structs fields in sync.
Shd we rename ClusterName to Key and also drop SourceKey to Key ?
SourceBinding.Key and ClusterBindings.Key avoids stuttering.

        clusterBindings:
          -Key: test

Can we consider renaming ClusterBinding.InputName and SourceBinding.Name to something like BoundTo.
We can also define a struct Binding and then include common fields in those.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Shd we rename ClusterName to Key and also drop SourceKey to Key ?

I agree with you to stay in sync across these bindings to make the pipeline yaml more readable. I like key for bindings. 👍

Can we consider renaming ClusterBinding.InputName and SourceBinding.Name to something like BoundTo

I think keyword bindings in ClusterBindings/ SourceBindings convey that this is templated to an another definition(like task). I think BoundTo might be redundant.

How about explicitly calling out taskInputName(since both are bound to tasks) or keep the original key inputName (more generic)?

Copy link
Member

Choose a reason for hiding this comment

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

agreed key is a better name the key field

  • ClusterBinding.InputName becomes ClusterBinding.Key
  • SourceBinding.SourceKey becomes SourceBinding.Key (think this should be a separate PR since this PR here is about adding ClusterBinding)

But since we already have the type name including the word Binding I feel the field name should be enough to clarify what it is

eg:

apiVersion: pipeline.knative.dev/v1beta1
kind: Pipeline
metadata:
  name: wizzbang-pipeline
  namespace: default
spec:
    tasks:
      - name: 'test'
        taskRef:
          name: test-wizzbang-task
        inputsourceBindings:
          - name: repoUnderTest
            key: wizzbang   <- changed to key in different PR
        clusterBindings:
          - name: test-cluster  <- name of this cluster binding
            key: gke-cluster-27    <- key to refer to
    .........

@knative-prow-robot knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Sep 24, 2018
Shash Reddy added 6 commits September 24, 2018 14:36
- Add clusters to pipelineparams
- Add cluster inputs for tasks
- Add clusterBindings for Pipelines
@tejal29
Copy link
Contributor

tejal29 commented Sep 24, 2018

/lgtm

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 24, 2018
@nader-ziada
Copy link
Member

/lgtm as well

@bobcatfish can you please take a look and merge if you think its all good, trying to get the PRs in before #57 makes big changes to the repo to reduce conflicts

@tejal29 tejal29 merged commit 17d92ee into tektoncd:master Sep 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants