-
Notifications
You must be signed in to change notification settings - Fork 73
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
sync REST parameters according to gRPC definitions #554
base: main
Are you sure you want to change the base?
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
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 Once the patch is verified, the new status will be reflected by the 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. |
/kind documentation |
cc @xinnjie |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/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>
The difficulty for this PR change is that words like For example we could say Sticking on using the short name in openAPI doc could be more intuitive IMO. Just calling |
But For reference, here in this definition results/proto/v1alpha2/api.proto Line 40 in 1de371e
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 Lines 213 to 217 in 1de371e
|
Sorry, didn't notice that you have added extra decription about But I am not sure about whether
I think we should not say Lines 213 to 217 in 1de371e
Anyway, in my very personal opinion, (please notice that there are little discription about terminology like |
@@ -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://github.com/raw/tektoncd/results/main/docs/api/openapi.yaml). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An interesting thought - running our own Swagger instance within https://tekton.dev/docs/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I was thinking of proposing this in our next WG call. Let me add it.
paths: | ||
/v1alpha2/parents/{parent}/results: | ||
summary: List Results | ||
/v1alpha2/parents/{log-name}: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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. |
Could be useful when we are discussing this topic. Current version:
Problem:
The DEVELOPMENT.md example:
Candidate version 1:
Probelm:Lost the infomation about connection between Candidate version 2:
ProbelmThe term Maybe we could call Extra thoughtAPI definition and implementation could be improved.
When creating result, We could set |
is WG call on Aug 17, 20:30 (UTC +8) ? And at this meeting? |
Yes @xinnjie |
@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. |
Changes
UID
description aboutResult
API #533Test
Verify at https://petstore.swagger.io/?url=https://github.com/raw/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:
/kind <type>
. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tepRelease Notes