search

tektoncd / results

Long term storage of execution results.
Apache License 2.0
78 stars 74 forks source link

sync REST parameters according to gRPC definitions #554

Open avinal opened 1 year ago

avinal commented 1 year ago

Changes

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:

Release Notes

NONE
tekton-robot commented 1 year ago

[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](https://github.com/tektoncd/results/blob/main/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 commented 1 year ago

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](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
avinal commented 1 year ago

/kind documentation

avinal commented 1 year ago

cc @xinnjie

xinnjie commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

avinal commented 1 year ago

Yes @xinnjie

tekton-robot commented 1 year ago

@avinal: PR needs rebase.

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.