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

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

Merged
merged 1 commit into from
Sep 27, 2022

Conversation

chuangw6
Copy link
Member

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

@tekton-robot tekton-robot added the release-note-none Denotes a PR that doesnt merit a release note. label Sep 16, 2022
@tekton-robot tekton-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Sep 16, 2022
@chuangw6
Copy link
Member Author

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

@chuangw6
Copy link
Member Author

/kind doc

@tekton-robot
Copy link
Collaborator

@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.

@chuangw6
Copy link
Member Author

/kind documentation

@tekton-robot tekton-robot added the kind/documentation Categorizes issue or PR as related to documentation. label Sep 16, 2022
@abayer
Copy link
Contributor

abayer commented Sep 19, 2022

/approve

@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 19, 2022
@lbernick
Copy link
Member

lbernick commented Sep 19, 2022

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.

@chuangw6
Copy link
Member Author

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?

@lbernick
Copy link
Member

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".

@chuangw6 chuangw6 force-pushed the update-api-spec branch 2 times, most recently from a9b7a9a to d7e80a3 Compare September 19, 2022 14:50
@chuangw6
Copy link
Member Author

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

docs/api-spec.md Outdated Show resolved Hide resolved
docs/api-spec.md Outdated Show resolved Hide resolved
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]>

### `TaskRunResult`

| Field Name | Field Type | Requirement |
|------------|------------|-------------|
| `name` | string | REQUIRED |
| `value` | string | REQUIRED |
| `type` | Enum:<br>- `"string"` (default)<br>- `"array"` <br>- `"object"` | RECOMMENDED (Each of the values is RECOMMENDED.) |
| `value` | `ParamValue` | REQUIRED |
Copy link
Member

Choose a reason for hiding this comment

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

it's a bit unfortunate that we're changing the type of a required field but since we only have one primary implementation of tekton currently it's probably OK?

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess so because it's a backward compatible change. String type result is still supported as the way it was supported before.

Copy link
Member

Choose a reason for hiding this comment

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

Just to clarify, if there were other implementations of the Tekton API, this would be a problem. The reason is that they might support the field "value" as a string, but with this change they are "required" to support value as a "ParamValue". This would mean an implementation that was previously conformant would not be conformant after this change. But since we have one main implementation, it doesn't matter too much.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the clarification!

@tekton-robot
Copy link
Collaborator

[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:

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

@lbernick
Copy link
Member

/lgtm since andrew previously approved.

@lbernick
Copy link
Member

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 27, 2022
@chuangw6
Copy link
Member Author

/test check-pr-has-kind-label

@tekton-robot
Copy link
Collaborator

@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.

@chuangw6
Copy link
Member Author

/retest

@tekton-robot tekton-robot merged commit fe52023 into tektoncd:main Sep 27, 2022
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. kind/documentation Categorizes issue or PR as related to documentation. lgtm Indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesnt merit a release note. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants