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

[CI:DOCS] swagger: add operationIds that match with docker #9123

Closed
wants to merge 2 commits into from

Conversation

tmds
Copy link
Contributor

@tmds tmds commented Jan 27, 2021

@jwhonce @baude ptal.

I only added operationIds to to match with the docker ones.
You may want to add operationIds to the operations specific to podman too.

@tmds tmds force-pushed the swagger_docker_ids branch from 0eedab6 to f8c1b1a Compare January 27, 2021 15:04
@mheon
Copy link
Member

mheon commented Jan 27, 2021

I think you need to add [CI:DOCS] to the title to disable full CI, since this is a docs-only change. You will probably need a force-push after that.

Otherwise LGTM

@mheon
Copy link
Member

mheon commented Jan 27, 2021

/approve

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mheon, tmds

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 27, 2021
@jwhonce jwhonce changed the title swagger: add operationIds that match with docker [CI:DOCS] swagger: add operationIds that match with docker Jan 27, 2021
@jwhonce
Copy link
Member

jwhonce commented Jan 27, 2021

@tmds If you add [NO TESTS NEEDED] to your commit message, the merge bot will not flag your PR as missing tests.

@baude This PR does show an issue where we have an operationalId conflict between the libpod API and the docker API, SystemVersion is duplicated. Opinion?

@tmds
Copy link
Contributor Author

tmds commented Jan 28, 2021

@baude This PR does show an issue where we have an operationalId conflict between the libpod API and the docker API, SystemVersion is duplicated. Opinion?

@jwhonce I don't see how there is a duplicate. It looks like distinct swagger sections?

        // swagger:operation GET /version compat CompatSystemVersion
        // ---
        // summary: Component Version information
+       // operationId: SystemVersion
        // tags:
        // - system (compat)
        // produces:
        // - application/json
        // responses:
        //   200:
        //    $ref: "#/responses/Version"
        r.Handle("/version", s.APIHandler(compat.VersionHandler)).Methods(http.MethodGet)
        r.Handle(VersionedPath("/version"), s.APIHandler(compat.VersionHandler)).Methods(http.MethodGet)
        // swagger:operation GET /libpod/version libpod SystemVersion

It does look like there is a duplicate for ContainerChanges:

        // swagger:operation GET /containers/{name}/changes libpod libpodChangesContainer
        // swagger:operation GET /libpod/containers/{name}/changes compat changesContainer
        // ---
        // tags:
        //   - containers
        //   - containers (compat)
        // summary: Report on changes to container's filesystem; adds, deletes or modifications.
+       // operationId: ContainerChanges
        // description: |

Is the fix to have separate swagger sections for these operations, and only include the operationId in one?

@tmds
Copy link
Contributor Author

tmds commented Feb 2, 2021

@jwhonce can you take a look at my previous comment?

@jwhonce
Copy link
Member

jwhonce commented Feb 2, 2021

@tmds I saw the conflict between your PR and https://github.com/containers/podman/blob/master/pkg/api/server/register_version.go#L23 regarding the operationId SystemVersion .

Since we have endpoints for the compatibility and libpod in one API we need to namespace them. To fix this across the API, we would need to remove Compat from the Compat on the swagger:operation lines and add Libpod to the libpod operations. If we make those changes, then the swagger processor should gives us your proposed operationId's without having to add operationId to override the one given on the swagger:operation line.

/cc @baude @rhatdan

@tmds
Copy link
Contributor Author

tmds commented Feb 3, 2021

@jwhonce based on your comment, I've moved the operationIds to the end of the swagger:operation lines. Let me know if I should make additional changes.

@tmds
Copy link
Contributor Author

tmds commented Feb 3, 2021

Since we have endpoints for the compatibility and libpod in one API we need to namespace them. To fix this across the API, we would need to remove Compat from the Compat on the swagger:operation lines and add Libpod to the libpod operations. If we make those changes

Thinking out loud: you may want to generate separate swagger files, one for libpod, and one for compat. Both of them can use the same operationIds.

@jwhonce
Copy link
Member

jwhonce commented Feb 4, 2021

@tmds I need to think on that suggestion.

Immediate ideas:

  • It would fix this issue and supporting /swagger and /libpod/swagger endpoints is trivial.
  • Maintenance would be a bit painful since "like" endpoints would be in different files, /containers/json vs /libpod/containers/json
  • Generating a client using both API's would bring us back to this issue. Is that a real concern?

/cc @mheon @baude

@vrothberg
Copy link
Member

Friendly ping

@github-actions
Copy link

A friendly reminder that this PR had no activity for 30 days.

@rhatdan
Copy link
Member

rhatdan commented Mar 29, 2021

@jwhonce @baude @mheon What do we want to do with this?

@jwhonce
Copy link
Member

jwhonce commented Apr 5, 2021

@rhatdan See #9944

jwhonce added a commit to jwhonce/podman that referenced this pull request Apr 6, 2021
Libpod operation id's changed to better match compatibile id

Builds on containers#9123 and corrects
a duplicated ID.

Signed-off-by: Jhon Honce <[email protected]>
@jwhonce
Copy link
Member

jwhonce commented Apr 6, 2021

@tmds I included your commits in #9944 and rebased to master.

@jwhonce jwhonce closed this Apr 6, 2021
mheon pushed a commit to mheon/libpod that referenced this pull request Apr 16, 2021
Libpod operation id's changed to better match compatibile id

Builds on containers#9123 and corrects
a duplicated ID.

Signed-off-by: Jhon Honce <[email protected]>
jmguzik pushed a commit to jmguzik/podman that referenced this pull request Apr 26, 2021
Libpod operation id's changed to better match compatibile id

Builds on containers#9123 and corrects
a duplicated ID.

Signed-off-by: Jhon Honce <[email protected]>
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 23, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants