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.

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

docs: finish pebble http api specs #544

Merged
merged 4 commits into from
Jan 2, 2025

Conversation

IronCore864
Copy link
Contributor

@IronCore864 IronCore864 commented Dec 27, 2024

In this PR:

Refactor

  • Add 502 responses for /v1/health endpoint according to David's suggestion, because otherwise, people would think it's 200 even if healthy is false.
  • Reformat multi-line descriptions.
  • Change "examples" to "example" to make the spec shorter with less indentation
  • Change the example to JSON format to make it less error-prone (no need to translate JSON response to YAML format in the spec).
  • Rename component names to more appropriate ones because they will appear in the schema. Convention: VerbResource[Description]Response, for example, GetPlanResponse, GetHealthOKResponse.

Newly added specs

  • /v1/plan, code.
  • /v1/layers, code.
  • /v1/logs, code.
  • /v1/signals, code.
  • /v1/files, code: GET, POST.
  • /v1/tasks/{task-id}/websocket/{websocket-id}, code.
  • /v1/exec, code.
  • /v1/notices, code: GET, POST.
  • /v1/notices/{id}, code.
  • /v1/identities, code: GET, POST.

Preview: https://canonical-pebble--544.com.readthedocs.build/en/544/reference/api/.

@IronCore864 IronCore864 requested a review from dwilding December 27, 2024 14:48
@IronCore864 IronCore864 marked this pull request as ready for review December 27, 2024 14:54
docs/specs/openapi.yaml Outdated Show resolved Hide resolved
docs/specs/openapi.yaml Outdated Show resolved Hide resolved
docs/specs/openapi.yaml Outdated Show resolved Hide resolved
docs/specs/openapi.yaml Outdated Show resolved Hide resolved
docs/specs/openapi.yaml Outdated Show resolved Hide resolved
docs/specs/openapi.yaml Outdated Show resolved Hide resolved
docs/specs/openapi.yaml Outdated Show resolved Hide resolved
docs/specs/openapi.yaml Outdated Show resolved Hide resolved
docs/specs/openapi.yaml Outdated Show resolved Hide resolved
docs/specs/openapi.yaml Outdated Show resolved Hide resolved
docs/specs/openapi.yaml Outdated Show resolved Hide resolved
@dwilding
Copy link
Contributor

Thank you for preparing all of this! I've finished my review and have added comments on specific parts.

Some other comments:

  • It looks like one examples still needs changing to example. The one under POST /v1/services

  • Most descriptions have a period at the end, but a few are missing a period. I won't list them all here, but I used this regex to find them: description:.*[^"|\.]$

  • A few 200 descriptions start with "Get ...". I think it would be clearer to describe the responses differently, as "Get" is more appropriate for a description of a request. How about these changes:

    • Get plan. ==> The current plan in YAML format.
    • Get service logs in JSON lines format. ==> Service logs in JSON lines format.
    • Get checks ==> Information about health checks.
    • Get changes ==> Information about changes.
  • GET /v1/files appears more complicated than necessary because the operation does two different things, depending on the value of action. Would it be possible to document this as two separate endpoints?

    • GET /v1/files?action=list
    • GET /v1/files?action=read

    But if this is too messy to achieve in OpenAPI, I think it's OK to leave it as-is.

@IronCore864
Copy link
Contributor Author

Thank you for preparing all of this! I've finished my review and have added comments on specific parts.

Some other comments:

  • It looks like one examples still needs changing to example. The one under POST /v1/services

  • Most descriptions have a period at the end, but a few are missing a period. I won't list them all here, but I used this regex to find them: description:.*[^"|\.]$

  • A few 200 descriptions start with "Get ...". I think it would be clearer to describe the responses differently, as "Get" is more appropriate for a description of a request. How about these changes:

    • Get plan. ==> The current plan in YAML format.
    • Get service logs in JSON lines format. ==> Service logs in JSON lines format.
    • Get checks ==> Information about health checks.
    • Get changes ==> Information about changes.
  • GET /v1/files appears more complicated than necessary because the operation does two different things, depending on the value of action. Would it be possible to document this as two separate endpoints?

    • GET /v1/files?action=list
    • GET /v1/files?action=read

    But if this is too messy to achieve in OpenAPI, I think it's OK to leave it as-is.

I fixed most comments above. There was a human error (the example indentation), and other style improvement suggestions were all adopted.

There is one thing I did not change, which is the example for notices, it doesn't have repeat-after in the response because I could not get this from the Pebble server no matter with custom notice or change-update notice.

For the three points above:

  1. The last "examples" under POST /v1/services: I kept it because I want to have a named example, the name shows on the rendered page. But after a second thought, I think consistency is better, so your suggestions above are also taken.

  2. The 200 response description: Yes your proposal makes sense and I updated all of them.

  3. On separating /v1/files into two endpoints, after some research, I did not find a very good solution.

I think if we can split the API into two, following RestAPI design, it might be better, like /v1/files/{filepath} for reading the file and /v1/files for list files. But we probably can't do this change because it's not backward compatible.

If we are not changing the URL, potentially, we could unify the response format. Even though the data returned is different, we can encapsulate it within JSON for both actions, for example:

examples:
  list:
    value:
      {
        "type": "sync",
        "status-code": 200,
        "status": "OK",
        "result": {
          "action": "list",
          "files": [
            { "path": "/file1.txt", ... }
          ]
        }
      }
  read:
    value:
      {
        "type": "sync",
        "status-code": 200,
        "status": "OK",
        "result": {
          "action": "read",
          "files": [
            { "path": "/file1.txt", "content": "File content here", ... }
          ]
        }
      }

But this is also not backward compatible.

Given the current situation, I will have to leave it as it is.

@IronCore864 IronCore864 merged commit 4f96724 into canonical:master Jan 2, 2025
18 checks passed
@IronCore864 IronCore864 deleted the http-api-spec branch January 2, 2025 07:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants