sync REST parameters according to gRPC definitions #554

avinal · 2023-08-08T10:19:18Z

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 OpenAPI misleading UID description about ResultAPI #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:

Has Docs included if any changes are user facing
Has Tests included if any functionality added or changed
Tested your changes locally (if this is a code change)
Follows the commit message standard
Meets the Tekton contributor standards (including functionality, content, code)
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
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 · 2023-08-08T10:19:20Z

[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 · 2023-08-08T10:19:27Z

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 · 2023-08-08T10:20:37Z

/kind documentation

avinal · 2023-08-08T10:21:12Z

cc @xinnjie

khrm

/ok-to-test

- remove uid from parameter, replace with _-name instead - fix openapi syntax warnings - remove rest-api markdown docs - fixes tektoncd#533 Signed-off-by: Avinal Kumar <avinal@redhat.com>

xinnjie · 2023-08-09T16:23:12Z

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 · 2023-08-09T17:24:31Z

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

results/proto/v1alpha2/api.proto

Line 40 in 1de371e

patch: "/apis/results.tekton.dev/v1alpha2/parents/{result.name=*/results/*}"

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

results/docs/DEVELOPMENT.md

Lines 213 to 217 in 1de371e

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

xinnjie · 2023-08-10T08:18:29Z

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

results/docs/DEVELOPMENT.md

Lines 213 to 217 in 1de371e

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

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.

adambkaplan · 2023-08-10T20:04:05Z

docs/DEVELOPMENT.md

@@ -189,7 +189,11 @@ is because we do not have health checking set up yet. Please refer <https://gith

 ### Using `curl` + `kubectl port-forward`

-See the available REST endpoints in the [OpenAPI specification](api/rest-api-spec.md) docs. The API request URL is of the format `https://{server_url}/apis/results.tekton.dev/v1alpha2/parents/{name/path-to-the-resource}`. For debugging server_url is `localhost:port-exposed`.
+See the available REST endpoints in
+the [OpenAPI specification](https://petstore.swagger.io/?url=https://raw.githubusercontent.com/tektoncd/results/main/docs/api/openapi.yaml).


An interesting thought - running our own Swagger instance within https://tekton.dev/docs/

Yes, I was thinking of proposing this in our next WG call. Let me add it.

adambkaplan · 2023-08-10T20:11:38Z

docs/api/openapi.yaml

 paths:
-  /v1alpha2/parents/{parent}/results:
-    summary: List Results
+  /v1alpha2/parents/{log-name}:


🤔 I see why it is rendering like this. The protobuf spec for the "name" includes a pattern match that nests pretty deep: https://github.com/tektoncd/results/blob/main/proto/v1alpha2/api.proto#L99. Fixing this to be more REST-ful would break the protobuf API.

We are ultimately arguing because the term "name" is way too overloaded. Are we talking about a Kubernetes object name? A record identifier in the Tekton Results database?

@avinal are you still writing this by hand? Or are you generating the openapi YAML?

True, the term name is too overloaded, and I don't see a legitimate way of simplifying. @xinnjie, if you can join our next WG call, we can have a discussion and agree on an approach to remove this ambiguity.

are you still writing this by hand? Or are you generating the openapi YAML?

Sadly yes, existing solutions don't work great or too complex for our use case. I am thinking of taking inspiration and then implement our own.

avinal · 2023-08-11T06:35:09Z

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 · 2023-08-17T06:46:15Z

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 · 2023-08-17T06:49:31Z

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

avinal · 2023-08-17T09:09:51Z

Yes @xinnjie

tekton-robot · 2023-10-31T14:26:52Z

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

tekton-robot added the release-note-none Denotes a PR that doesnt merit a release note. label Aug 8, 2023

tekton-robot requested review from alan-ghelardi and dibyom August 8, 2023 10:19

tekton-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 8, 2023

tekton-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Aug 8, 2023

tekton-robot added the kind/documentation Categorizes issue or PR as related to documentation. label Aug 8, 2023

khrm reviewed Aug 9, 2023

View reviewed changes

tekton-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 9, 2023

avinal force-pushed the openapi-fixes branch from 349ec58 to 61bccb6 Compare August 9, 2023 09:36

sync REST parameters according to gRPC definitions

c786052

- remove uid from parameter, replace with _-name instead - fix openapi syntax warnings - remove rest-api markdown docs - fixes tektoncd#533 Signed-off-by: Avinal Kumar <avinal@redhat.com>

avinal force-pushed the openapi-fixes branch from 61bccb6 to c786052 Compare August 9, 2023 09:54

adambkaplan reviewed Aug 10, 2023

View reviewed changes

tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 31, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sync REST parameters according to gRPC definitions #554

sync REST parameters according to gRPC definitions #554

avinal commented Aug 8, 2023 •

edited

Loading

tekton-robot commented Aug 8, 2023

tekton-robot commented Aug 8, 2023

avinal commented Aug 8, 2023

avinal commented Aug 8, 2023

khrm left a comment

xinnjie commented Aug 9, 2023

avinal commented Aug 9, 2023

xinnjie commented Aug 10, 2023

adambkaplan Aug 10, 2023

avinal Aug 11, 2023

adambkaplan Aug 10, 2023

avinal Aug 11, 2023

avinal commented Aug 11, 2023

xinnjie commented Aug 17, 2023

xinnjie commented Aug 17, 2023

avinal commented Aug 17, 2023

tekton-robot commented Oct 31, 2023

sync REST parameters according to gRPC definitions #554

Are you sure you want to change the base?

sync REST parameters according to gRPC definitions #554

Conversation

avinal commented Aug 8, 2023 • edited Loading

Changes

Test

Submitter Checklist

Release Notes

tekton-robot commented Aug 8, 2023

tekton-robot commented Aug 8, 2023

avinal commented Aug 8, 2023

avinal commented Aug 8, 2023

khrm left a comment

Choose a reason for hiding this comment

xinnjie commented Aug 9, 2023

avinal commented Aug 9, 2023

xinnjie commented Aug 10, 2023

adambkaplan Aug 10, 2023

Choose a reason for hiding this comment

avinal Aug 11, 2023

Choose a reason for hiding this comment

adambkaplan Aug 10, 2023

Choose a reason for hiding this comment

avinal Aug 11, 2023

Choose a reason for hiding this comment

avinal commented Aug 11, 2023

xinnjie commented Aug 17, 2023

Current version:

Problem:

Candidate version 1:

Probelm:

Candidate version 2:

Probelm

Extra thought

xinnjie commented Aug 17, 2023

avinal commented Aug 17, 2023

tekton-robot commented Oct 31, 2023

avinal commented Aug 8, 2023 •

edited

Loading