pipeline icon indicating copy to clipboard operation
pipeline copied to clipboard

Update API spec doc to reflect the new fields introduced from TEP75&76

Open chuangw6 opened this issue 3 years ago • 10 comments

Changes

Implementation of TEP75&76 supported object type param, object type result and array type result, which introduced some changes to the API spec i.e. ParamSpec, TaskResult and TaskRunResult.

This PR updates the doc to reflect the changes.

Signed-off-by: Chuang Wang [email protected]

Submitter Checklist

As the author of this PR, please check off the items in this checklist:

  • [ ] Has Docs included if any changes are user facing
  • [ ] Has Tests included if any functionality added or changed
  • [ ] Follows the commit message standard
  • [ ] Meets the Tekton contributor standards (including functionality, content, code)
  • [ ] Has a kind label. You can add one by adding a comment on this PR that contains /kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tep
  • [ ] Release notes block below has been updated with any user facing changes (API changes, bug fixes, changes requiring upgrade notices or deprecation warnings)
  • [ ] Release notes contains the string "action required" if the change requires additional action from users switching to the new release

Release Notes

NONE

chuangw6 avatar Sep 16 '22 19:09 chuangw6

@Yongxuanzhang Please take a look. Let me know if I miss anything. Thanks

chuangw6 avatar Sep 16 '22 19:09 chuangw6

/kind doc

chuangw6 avatar Sep 16 '22 19:09 chuangw6

@chuangw6: The label(s) kind/doc cannot be applied, because the repository doesn't have them.

In response to this:

/kind doc

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 avatar Sep 16 '22 19:09 tekton-robot

/kind documentation

chuangw6 avatar Sep 16 '22 19:09 chuangw6

/approve

abayer avatar Sep 19 '22 13:09 abayer

I don't think we want to mark array results and object params + results as required for conformance especially since these fields are still in alpha. TEP-0012 says that we may add "an optional process to very mature supported features, to potentially mark them as required by the spec", and TEP-0033 states that "Fields should not be added to the Tekton conformance surface until they are promoted to the same stability level as the CRD impacted". This doc also states that "In general a resource or field should not be added as REQUIRED directly, as this may cause unsuspecting previously-conformant implementations to suddenly no longer be conformant". Maybe "recommended" would be better.

lbernick avatar Sep 19 '22 13:09 lbernick

I don't think we want to mark array results and object params + results as required for conformance especially since these fields are still in alpha. TEP-0012 says that we may add "an optional process to very mature supported features, to potentially mark them as required by the spec", and TEP-0033 states that "Fields should not be added to the Tekton conformance surface until they are promoted to the same stability level as the CRD impacted". Maybe "recommended" would be better.

Thanks for the pointers @lbernick ! We are not marking specifically array result, object param + result as required. It's the type field. And I agree that this is indeed not a required field since the mutating webhook will set it if not provided. In addition, I feel like some other fields marked as required should also be changed to recommended or optional i.e. description, paramSpec's default. wdyt?

chuangw6 avatar Sep 19 '22 14:09 chuangw6

Thanks for the pointers @lbernick ! We are not marking specifically array result, object param + result as required. It's the type field.

If supporting "type" is required, doesn't that imply that supporting object params + results is required? It wouldn't make sense to allow users to define a "type" but have it not do anything. Unless the only "type" value that is required to be supported is type string? This is making me realize that for enum fields, we probably need to specify which values of the enum should be supported, rather than specifying conformance requirements for the field as a whole.

And I agree that this is indeed not a required field since the mutating webhook will set it if not provided.

This is a detail of our current implementation of the Tekton API, but the conformance spec dictates what fields all implementations must support.

In addition, I feel like some other fields marked as required should also be changed to recommended or optional i.e. description, paramSpec's default. wdyt?

I think "description" and "default" have been around for much longer and we are much more confident in the stability of those fields. Since we already have these fields as required, I don't see a good reason to downgrade them to "recommended".

lbernick avatar Sep 19 '22 14:09 lbernick

@lbernick I see. I've updated to mark the type field in TaskResult and TaskRunResult as recommended for the object and array. PTAL! Thanks

chuangw6 avatar Sep 19 '22 14:09 chuangw6

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abayer, lbernick

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:
  • ~~OWNERS~~ [abayer,lbernick]

Approvers can indicate their approval by writing /approve in a comment Approvers can cancel approval by writing /approve cancel in a comment

tekton-robot avatar Sep 19 '22 15:09 tekton-robot

/lgtm since andrew previously approved.

lbernick avatar Sep 22 '22 20:09 lbernick

/lgtm

lbernick avatar Sep 27 '22 19:09 lbernick

/test check-pr-has-kind-label

chuangw6 avatar Sep 27 '22 20:09 chuangw6

@chuangw6: The specified target(s) for /test were not found. The following commands are available to trigger required jobs:

  • /test pull-tekton-pipeline-alpha-integration-tests
  • /test pull-tekton-pipeline-build-tests
  • /test pull-tekton-pipeline-integration-tests
  • /test tekton-pipeline-unit-tests

The following commands are available to trigger optional jobs:

  • /test pull-tekton-pipeline-go-coverage

Use /test all to run all jobs.

In response to this:

/test check-pr-has-kind-label

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 avatar Sep 27 '22 20:09 tekton-robot

/retest

chuangw6 avatar Sep 27 '22 20:09 chuangw6