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 description field to Spec #2089

Merged
merged 1 commit into from
Mar 9, 2020
Merged

Conversation

piyush-garg
Copy link
Contributor

@piyush-garg piyush-garg commented Feb 24, 2020

This will add description field to spec
of pipeline, task, clustertask, resource and condition

This can be used to provide description and
can further used in CLI and UI.

Add docs and tests

Fix #993

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

See the contribution guide for more details.

Double check this list of stuff that's easy to miss:

Reviewer Notes

If API changes are included, additive changes must be approved by at least two OWNERS and backwards incompatible changes must be approved by more than 50% of the OWNERS, and they must first be added in a backwards compatible way.

Release Notes

Add an optional description field to spec of pipeline, task, clustertask, resource and condition.

@googlebot googlebot added the cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit label Feb 24, 2020
@tekton-robot tekton-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Feb 24, 2020
@piyush-garg
Copy link
Contributor Author

@piyush-garg
Copy link
Contributor Author

/cc @vdemeester @bobcatfish @siamaksade

@tekton-robot
Copy link
Collaborator

@piyush-garg: GitHub didn't allow me to request PR reviews from the following users: siamaksade.

Note that only tektoncd members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @vdemeester @bobcatfish @siamaksade

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.

@vdemeester
Copy link
Member

So initially I thought we could use annotation (and a standard tekton.dev/description key) to have the description, but I am also fine having the description field. I wonder how other project tend to do this.

@dibyom
Copy link
Member

dibyom commented Feb 25, 2020

I think we have a Description field for Params already.
@piyush-garg would be good to mention this in the docs syntax section so that this is easily discoverable. Also a quick unit test would be nice :D

@vdemeester
Copy link
Member

To give more on my thought of re-use annotation for this, it could look like the following

apiVersion: tekton.dev/v1alpha1
kind: Task
metadata:
  name: print-date
  annotations:
    description: |
      A simple task that prints the date
# […]
---
# OR
apiVersion: tekton.dev/v1alpha1
kind: Task
metadata:
  name: golang-build
  annotations:
    tekton.dev/description: |
      A description here
spec:
  inputs:
    params:
    - name: package
      description: base package to build in
  steps:
  - name: build
    image: golang:$(inputs.params.version)
    workingdir: /workspace/src/$(inputs.params.package)
    # […]

Few benefits:

  • It doesn't require changes in the API.
  • I feel it's a good use case of k8s annotations :upside_down_face:
  • As with istio guidelines on app and other labels/annotation, maybe description can be an annotation key that a lot of k8s project share and use.

@piyush-garg
Copy link
Contributor Author

Considering that validation can be done in a better strong way when using a spec field, and also to keep consistency with params and resource declaration, I will prefer to have Description field in spec, and it is optional so will not break anything on api-level

@piyush-garg
Copy link
Contributor Author

@bobcatfish @dibyom @vdemeester Any final thoughts on this so that I can work further on this. Thanks

@afrittoli
Copy link
Member

I don't have strong feelings about this, I would tend towards annotations since even if description is missing / not validated it does not have impact on functionality.

One thing I do care about is the ability for tasks to self-document themselves, and to render a folder of tasks into a browsable documentation. But I believe neither of the options prevent this from happening.

@dibyom
Copy link
Member

dibyom commented Mar 2, 2020

Don't have strong feelings about this either....let's go with the annotation approach for now?

@piyush-garg
Copy link
Contributor Author

piyush-garg commented Mar 3, 2020

Thanks, @vdemeester @dibyom @afrittoli for the suggestion, I will add docs regarding the annotation and no struct changes are required.

I will mention the annotation key tekton.dev/description to be used for description.

Should I also add a default empty annotation in the case where the key is not there in yaml?

@tekton-robot tekton-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Mar 3, 2020
@piyush-garg piyush-garg changed the title Add description field to spec Add information about description annotation Mar 3, 2020
@tekton-robot
Copy link
Collaborator

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/apis/annotation/annotation.go Do not exist 100.0%
pkg/apis/pipeline/v1alpha1/task_defaults.go 53.8% 50.0% -3.8
pkg/apis/pipeline/v1beta1/task_defaults.go 66.7% 50.0% -16.7

@piyush-garg
Copy link
Contributor Author

/test pull-tekton-pipeline-integration-tests

@tekton-robot
Copy link
Collaborator

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/apis/annotation/annotation.go Do not exist 100.0%
pkg/apis/pipeline/v1alpha1/task_defaults.go 53.8% 50.0% -3.8
pkg/apis/pipeline/v1beta1/task_defaults.go 66.7% 50.0% -16.7

@bobcatfish
Copy link
Collaborator

apologies for my late feedback but i added some comments to #993 (comment) - i not 100% sold on using an annotation vs using the spec, maybe there's something im missing tho! 🙏

/hold

@tekton-robot tekton-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 3, 2020
@tekton-robot tekton-robot removed the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 4, 2020
@tekton-robot tekton-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Mar 4, 2020
@piyush-garg piyush-garg changed the title Add information about description annotation Add description field to Spec Mar 4, 2020
@tekton-robot
Copy link
Collaborator

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
test/builder/pipeline.go 86.8% 87.0% 0.2
test/builder/task.go 69.7% 69.9% 0.2

@piyush-garg
Copy link
Contributor Author

@vdemeester @bobcatfish @dibyom I have updated the PR to add spec field after christie's comment. Please have a look at PR and let me know if something I have missed.

Copy link
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

/meow

@tekton-robot
Copy link
Collaborator

@vdemeester: cat image

In response to this:

/meow

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.

@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 6, 2020
@tekton-robot
Copy link
Collaborator

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
test/builder/pipeline.go 86.8% 87.0% 0.2
test/builder/task.go 69.7% 69.9% 0.2

@tekton-robot
Copy link
Collaborator

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
test/builder/pipeline.go 86.8% 87.0% 0.2
test/builder/task.go 69.7% 69.9% 0.2

docs/pipelines.md Outdated Show resolved Hide resolved
This will add description field to spec
of pipeline, task, clustertask, resource and condition

This can be used to provide description and
can further used in CLI and UI.

Add docs and tests

Fix tektoncd#993
@tekton-robot
Copy link
Collaborator

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
test/builder/pipeline.go 86.8% 87.0% 0.2
test/builder/task.go 69.7% 69.9% 0.2

@ghost
Copy link

ghost commented Mar 9, 2020

/lgtm

@tekton-robot tekton-robot assigned ghost Mar 9, 2020
@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 9, 2020
@tekton-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbwsg, vdemeester

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

@ghost
Copy link

ghost commented Mar 9, 2020

/hold cancel

@tekton-robot tekton-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 9, 2020
@tekton-robot tekton-robot merged commit a109d07 into tektoncd:master Mar 9, 2020
@vdemeester vdemeester added the needs-cherry-pick Indicates a PR needs to be cherry-pick to a release branch label Mar 9, 2020
@vdemeester vdemeester added this to the Pipelines 0.11 🐱 milestone Mar 9, 2020
@ghost ghost removed the needs-cherry-pick Indicates a PR needs to be cherry-pick to a release branch label Mar 9, 2020
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. cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Self describing Pipeline and Task Definitions
7 participants