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.

Sign up for GitHub

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

Jump to bottom

OpenAPI misleading UID description about ResultAPI #533

Open
xinnjie opened this issue Jul 19, 2023 · 10 comments · May be fixed by #554
Open

OpenAPI misleading UID description about ResultAPI #533

xinnjie opened this issue Jul 19, 2023 · 10 comments · May be fixed by #554
Assignees
@avinal
Labels
kind/documentation Categorizes issue or PR as related to documentation.

Comments

@xinnjie
Copy link
Contributor

xinnjie commented Jul 19, 2023 •
edited
Loading

Currently doc mix-use Result UID as both name and UID of result. The terminology should be sorted out, otherwise users may get confused.

About terminology

name of Result

Here result name refer to the name decided by the clients of Results API Server. And (from implementation aspect) currently Watcher use Taskrun/Pipelinerun UID as result name.

UID of Result

UID refer to the id decided by Results API Server when inserting Result object.

Actual Behavior

While GetResultRequest needs specifying result name to query Result, OpenAPI document that get /v1alpha2/parents/{parent}/results/{result_uid} get a single result given the UID.
There are also same wrong description about update, delete of Result

GRPC service definition:

rpc GetResult(GetResultRequest) returns (Result) {
  option (google.api.http) = {
    get: "/apis/results.tekton.dev/v1alpha2/parents/{name=*/results/*}"
  };
}

message GetResultRequest {
  string name = 1 [
    (google.api.field_behavior) = REQUIRED,
    (google.api.resource_reference) = {
      type: "tekton.results.v1alpha2/Result"
    }];
}

OpenAPI doc:

  /v1alpha2/parents/{parent}/results/{result_uid}:
    summary: Get, Create, Delete or update result
    get:
      tags:
        - Results
      responses:
        "200":
          content:
            application/json:
              schema:
                $ref: "#/components/schemas/Result"
          description: Default response
      operationId: get_result_by_uid
      summary: Get a single result given the UID
      description: ""

Expected Behavior

OpenAPI summary of get /v1alpha2/parents/{parent}/results/{result_uid} change Get a single result given the UID to Get a single result given name and so on.

Steps to Reproduce the Problem

Additional Info

  • Kubernetes version:

    Output of kubectl version:

    (paste your output here)

  • Tekton Pipeline version:

    Output of tkn version or kubectl get pods -n tekton-pipelines -l app=tekton-pipelines-controller -o=jsonpath='{.items[0].metadata.labels.version}'

  • Tekton Results version:
    current main branch
@xinnjie xinnjie added the kind/bug Categorizes issue or PR as related to a bug. label Jul 19, 2023
@xinnjie
Copy link
Contributor Author

xinnjie commented Jul 19, 2023

/kind documentation

@tekton-robot tekton-robot added the kind/documentation Categorizes issue or PR as related to documentation. label Jul 19, 2023
@xinnjie xinnjie changed the title OpenAPI wrong description about ResultsAPI OpenAPI misleading UID description about ResultAPI Jul 19, 2023
@xinnjie
Copy link
Contributor Author

xinnjie commented Jul 19, 2023

@enarha @sayan-biswas @khrm
Let's discuss about this documentation issue.

@avinal
Copy link
Member

avinal commented Jul 27, 2023

@xinnjie the difference between gRPC method and REST call. The gRPC method expects a single parameter called name which is of the pattern <parent_name>/results/<results_uid>. In the REST API call, I have made them separate parameters. I agree, it would be better to keep them in sync. Let me make the necessary changes.

@xinnjie
Copy link
Contributor Author

xinnjie commented Jul 27, 2023 •
edited
Loading

My point is that results_uid in <parent_name>/results/<results_uid> collides with uid in Result of Results Service API

message Result {
  string name = 1 [(google.api.resource_reference) = {
    child_type: "tekton.results.v1alpha2/Result"
  }];

  // Server assigned identifier of the Result.
  // DEPRECATED: use uid instead.
  string id = 2 [(google.api.field_behavior) = OUTPUT_ONLY, deprecated = true];
  // Server assigned identifier of the Result.
  string uid = 7 [(google.api.field_behavior) = OUTPUT_ONLY];
... 
}

Maybe we could call results_uid in <parent_name>/results/<results_uid> as results name, <parent_name>/results/<results_uid> as results full name.
Results UID is the UID in Resultof Results Service API.

Names could be discussed as long as long they do not collide.

@xinnjie
Copy link
Contributor Author

xinnjie commented Jul 27, 2023

/remove-kind bug

@tekton-robot tekton-robot removed the kind/bug Categorizes issue or PR as related to a bug. label Jul 27, 2023
@sayan-biswas
Copy link
Contributor

Agree with @xinnjie. result-name seems more accurate.

@avinal
Copy link
Member

avinal commented Jul 28, 2023

Okay let me open a fix.

@avinal
Copy link
Member

avinal commented Jul 31, 2023

/assign

avinal added a commit to avinal/tektoncd-results that referenced this issue Aug 8, 2023
@avinal
sync REST parameters according to gRPC definitions
349ec58 
- remove uid from parameter, replace with _-name instead
- fix openapi syntax warnings
- fixes tektoncd#533

Signed-off-by: Avinal Kumar <avinal@redhat.com>
@avinal avinal linked a pull request Aug 8, 2023 that will close this issue
Open
8 tasks
avinal added a commit to avinal/tektoncd-results that referenced this issue Aug 9, 2023
@avinal
sync REST parameters according to gRPC definitions
61bccb6 
- 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 added a commit to avinal/tektoncd-results that referenced this issue Aug 9, 2023
@avinal
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>
@tekton-robot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale with a justification.
Stale issues rot after an additional 30d of inactivity and eventually close.
If this issue is safe to close now please do so with /close with a justification.
If this issue should be exempted, mark the issue as frozen with /lifecycle frozen with a justification.

/lifecycle stale

Send feedback to tektoncd/plumbing.

@avinal
Copy link
Member

avinal commented Nov 8, 2023

/remove-lifecycle stale
Work is in progress

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@avinal avinal

Labels
kind/documentation Categorizes issue or PR as related to documentation.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

sync REST parameters according to gRPC definitions avinal/tektoncd-results
4 participants
@sayan-biswas @xinnjie @tekton-robot @avinal