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

APIv2: compatible api fixes #5410

Merged
merged 7 commits into from
Mar 6, 2020
Merged

APIv2: compatible api fixes #5410

merged 7 commits into from
Mar 6, 2020

Conversation

st1971
Copy link
Contributor

@st1971 st1971 commented Mar 5, 2020

Added addition API handler registration without the need for a API version number, this mimics the structure of the docker API..
Updated GetImages handler to remove null entry at the end of the images array.
Change GetContainers to map status of configured to created.
Implemented size parameter with default of false on ListContainers handler.
Implemented size parameter on GetContainer.

@mheon
Copy link
Member

mheon commented Mar 6, 2020

/approve

@jwhonce @baude PTAL... I don't know what we want to do about the duplicate endpoints with regard to swagger.

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 6, 2020
@st1971
Copy link
Contributor Author

st1971 commented Mar 6, 2020

@mheon did consider the swagger definition and i don't think it needs to change, the uris defined in swagger don't include the version information in the path, only taking about the docker compatible endpoints here and not the libpod ones

@@ -305,7 +305,8 @@ func GetImages(w http.ResponseWriter, r *http.Request) {
utils.Error(w, "Something went wrong.", http.StatusInternalServerError, errors.Wrap(err, "Failed get images"))
return
}
var summaries = make([]*handlers.ImageSummary, len(images)+1)
// +1 removed from len(images) as this leaves a null entry at the end of the image array
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for this comment. It will mean nothing in the future. Please remove the comment and just ad it to the PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed comment

imageId, imageName := l.Image()

var (
err error
sizeRootFs int64
sizeRW int64
state define.ContainerStatus
stateStr string
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't define it here. Just use stateStr=state.String() below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

return nil, err
}
} else {
sizeRW = 0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are defaults so no reason for this code block.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

if sizeRootFs, err = l.RootFsSize(); err != nil {
return nil, err
}
} else {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for else block, These are defaults.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The entire if sz block was wrong, have removed and changed the call to inspect to pass sz rather than it being hard coded to true, which seemed a better thing to do

@rhatdan
Copy link
Member

rhatdan commented Mar 6, 2020

Thanks @st1971 We need @jwhonce and @baude to review the API Changes.

st1971 added 2 commits March 6, 2020 13:17
removed defaulting of  query.Size
amended types.LibpodToContainer, removed hard coded true from inspect call

Signed-off-by: Steve Taylor <[email protected]>
Copy link
Member

@jwhonce jwhonce left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, Just need to restore one item.

@@ -305,7 +305,7 @@ func GetImages(w http.ResponseWriter, r *http.Request) {
utils.Error(w, "Something went wrong.", http.StatusInternalServerError, errors.Wrap(err, "Failed get images"))
return
}
var summaries = make([]*handlers.ImageSummary, len(images)+1)
var summaries = make([]*handlers.ImageSummary, len(images))
Copy link
Member

@jwhonce jwhonce Mar 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The +1 ensures we don't return an empty body (clients hate that :) ), when no images are found. So we either need to restore that, or handle that corner case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just spun up an f31 vm with moby-engine, with no images pulled down a curl to /images/json returns and empty array "[]", on podman in the same state if i put the +1 back in the a curl to /images/json returns "[null]" which is causing consuming code to fail. without the +1 podman produced the same as docker "[]".

without the +1 podman mimics the behavior of docker on this call

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@st1971 Okay, something must have changed in the response writer. Sorry for the noise, thanks for checking.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no worries

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jwhonce, mheon, st1971

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

@baude
Copy link
Member

baude commented Mar 6, 2020

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 6, 2020
@openshift-merge-robot openshift-merge-robot merged commit c3177b7 into containers:master Mar 6, 2020
@st1971 st1971 deleted the api-fixes branch March 7, 2020 17:26
@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 25, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 25, 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. lgtm Indicates that a PR is ready to be merged. 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.

7 participants