sync REST parameters according to gRPC definitions

[avinal]

Changes

• remove uid from parameter, replace with _-name instead
• fix openapi syntax warnings
• remove markdown docs for Openapi, difficult to maintain and did not server any real purpose.
• fixes #533

Test

Verify at https://petstore.swagger.io/?url=https://raw.githubusercontent.com/avinal/tektoncd-results/openapi-fixes/docs/api/openapi.yaml

Submitter Checklist

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

• [x] Has Docs included if any changes are user facing
• [ ] Has Tests included if any functionality added or changed
• [x] Tested your changes locally (if this is a code change)
• [x] Follows the commit message standard
• [x] Meets the Tekton contributor standards (including functionality, content, code)
• [x] Has a kind label. You can add a comment on this PR that contains /kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tep
• [x] 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 contain the string "action required" if the change requires additional action from users switching to the new release

Release Notes

NONE

[tekton-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: To complete the pull request process, please assign dibyom after the PR has been reviewed. You can assign the PR to them by writing /assign @dibyom in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

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

[tekton-robot]

Hi @avinal. Thanks for your PR.

I'm waiting for a tektoncd 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.

[avinal]

/kind documentation

[avinal]

cc @xinnjie

[xinnjie]

The difficulty for this PR change is that words like result name, record name, and log name are ambiguous.

For example we could say default/b959b3aa-55e8-4fb8-bc19-ce48bcfacce2 is the result name(in full name version), but we could also say b959b3aa-55e8-4fb8-bc19-ce48bcfacce2 is the result name(in short name version).

Sticking on using the short name in openAPI doc could be more intuitive IMO.

Just calling b959b3aa-55e8-4fb8-bc19-ce48bcfacce2, instead of default/b959b3aa-55e8-4fb8-bc19-ce48bcfacce2 , as result namemakes the Ωnew developers know result is named as <parent>/<name> instantly.

[avinal]

But b959b3aa-55e8-4fb8-bc19-ce48bcfacce2 is not the result's name it is the result's UID only. And to make sure that a person inputs a correct name always, I have added pattern matching for each of the name types.

For reference, here in this definition https://github.com/tektoncd/results/blob/1de371e2ebc5f5a5708c4ce2cdd8a654ef3a783a/proto/v1alpha2/api.proto#L40 the result.name is defined as */results/* which is of format parent-name/results/result-uid.

Furthermore, here in the example result output, we can clearly see the distinction https://github.com/tektoncd/results/blob/1de371e2ebc5f5a5708c4ce2cdd8a654ef3a783a/docs/DEVELOPMENT.md?plain=1#L213-L217

[xinnjie]

And to make sure that a person inputs a correct name always, I have added pattern matching for each of the name types.

Sorry, didn't notice that you have added extra decription about result-name here. It does make it clear in some way.

But I am not sure about whether /v1alpha2/parents/{log-name} is better than /v1alpha2/parents/{parent}/results/{result_uid}/logs/{log-uid}. Becuase at first sight, /v1alpha2/parents/{log-name} lost the infomation about connection between log and result. What's more, put /v1alpha2/parents/{log-name}, /v1alpha2/parents/{record-name}, and /v1alpha2/parents/{result-name} together, users may have doubt that why we can not distinguish resources by URL. It's not RESTful.

the result.name is defined as */results/* which is of format parent-name/results/result-uid.

I think we should not say */results/* is of format parent-name/results/result-uid. Because result-uid can be interpreted as 338481c9-3bc6-472f-9d1b-0f7705e6cb8c instead of 640d1af3-9c75-4167-8167-4d8e4f39d403 (the DEVELOPMENT.md example). https://github.com/tektoncd/results/blob/1de371e2ebc5f5a5708c4ce2cdd8a654ef3a783a/docs/DEVELOPMENT.md?plain=1#L213-L217

Anyway, in my very personal opinion, (please notice that there are little discription about terminology like result name and so on, I could be wrong likely) I like old style /v1alpha2/parents/{parent}/results/{result_uid}/logs/{log-uid} more, as long as {result_uid} could be renamed as result_name.

[avinal]

I think we should not say /results/ is of format parent-name/results/result-uid. Because result-uid can be interpreted as 338481c9-3bc6-472f-9d1b-0f7705e6cb8c instead of 640d1af3-9c75-4167-8167-4d8e4f39d403 (the DEVELOPMENT.md example).

I see, apologies for misunderstanding, I like the idea of short-name and full-name, but it would be nice to discuss this in the call.

[xinnjie]

Could be useful when we are discussing this topic.

Current version:

/v1alpha2/parents/{parent}/results/{result_uid}

Problem:

uidand name is mixed-up.

The DEVELOPMENT.md example:

  "results": [
    {
      "name": "default/results/640d1af3-9c75-4167-8167-4d8e4f39d403",
      "id": "338481c9-3bc6-472f-9d1b-0f7705e6cb8c",
      "uid": "338481c9-3bc6-472f-9d1b-0f7705e6cb8c",

uid should be interpreted as 338481c9-3bc6-472f-9d1b-0f7705e6cb8c instead of 640d1af3-9c75-4167-8167-4d8e4f39d403

Candidate version 1:

/v1alpha2/parents/{result-name} for result

/v1alpha2/parents/{log-name} for log

Probelm:

Lost the infomation about connection between log and result

Candidate version 2:

/v1alpha2/parents/{parent}/results/{name} for result

/v1alpha2/parents/{parent}/results/{result_name}/logs/{log_name} for log

Probelm

The term name is too overloaded. Result name could be default/results/640d1af3-9c75-4167-8167-4d8e4f39d403 and 640d1af3-9c75-4167-8167-4d8e4f39d403.

Maybe we could call default/results/640d1af3-9c75-4167-8167-4d8e4f39d403 as key or full name and 640d1af3-9c75-4167-8167-4d8e4f39d403 as short name

Extra thought

API definition and implementation could be improved.

CreateResultRequest definition:

message CreateResultRequest {
  // User provided parent to partition results under.
  string parent = 1 [
    (google.api.field_behavior) = REQUIRED,
    (google.api.resource_reference) = {
      type: "tekton.results.v1alpha2/Result"
    }];

  Result result = 2 [(google.api.field_behavior) = REQUIRED];
}

When creating result, parent is set as default, result.name is set as default/results/640d1af3-9c75-4167-8167-4d8e4f39d403. parent is just verification in Implementation ,

We could set parent is set as default and result.name as 640d1af3-9c75-4167-8167-4d8e4f39d403, then concate final DB key as default/results/640d1af3-9c75-4167-8167-4d8e4f39d403 . Then there is no overloaded usage about the term result name.

[xinnjie]

is WG call on Aug 17, 20:30 (UTC +8) ? And at this meeting? https://zoom.us/j/99187487778?pwd=UGZhOHd3cWJlVFhMTDNTVGxQeG1ndz09 @avinal

[avinal]

Yes @xinnjie

[tekton-robot]

@avinal: PR needs rebase.

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.