tektoncd / results

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

OpenAPI misleading `UID` description about `Result`API #533

Open xinnjie opened 1 year ago

xinnjie commented 1 year ago

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

xinnjie commented 1 year ago

/kind documentation

xinnjie commented 1 year ago

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

avinal commented 1 year ago

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

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

/remove-kind bug

sayan-biswas commented 1 year ago

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

avinal commented 1 year ago

Okay let me open a fix.

avinal commented 1 year ago

/assign

tekton-robot commented 1 year ago

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

/remove-lifecycle stale Work is in progress