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

fix reported compat issues #5554

Merged
merged 1 commit into from
Mar 19, 2020
Merged

Conversation

baude
Copy link
Member

@baude baude commented Mar 19, 2020

honor -1 in in list containers for compatibility mode. it is commonly used to indicate no limit.

change the json id parameter to Id in container create.

Fixes: #5553

Signed-off-by: Brent Baude [email protected]

honor -1 in in list containers for compatibility mode.  it is commonly used to indicate no limit.

change the json id parameter to Id in container create.

Fixes: containers#5553

Signed-off-by: Brent Baude <[email protected]>
@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: baude

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 Mar 19, 2020
@rhatdan
Copy link
Member

rhatdan commented Mar 19, 2020

LGTM

@rhatdan
Copy link
Member

rhatdan commented Mar 19, 2020

@mheon @jwhonce @edsantiago PTAL

@mheon
Copy link
Member

mheon commented Mar 19, 2020

/hold
/lgtm

@openshift-ci-robot openshift-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Mar 19, 2020
@TomSweeneyRedHat
Copy link
Member

LGTM

@max-arnold
Copy link

Are these other occurrencies of lowercase id also should beId?

libpod/pkg/api % grep -r '"id"' .
./handlers/compat/images.go:		Id             string            `json:"id"`
./handlers/compat/images.go:		Id             string            `json:"id"`
./handlers/types.go:	ID string `json:"id"`
./handlers/types.go:	ID string `json:"id"`
./handlers/types.go:	ID string `json:"id"`
./handlers/types.go:	ID             string `json:"id"`
./handlers/types.go:	ID string `json:"id"`
./handlers/types.go:	ID     string       `json:"id"`
./handlers/utils/containers.go:	ID string `json:"id"`

@baude
Copy link
Member Author

baude commented Mar 19, 2020

Are these other occurrencies of lowercase id also should beId?

libpod/pkg/api % grep -r '"id"' .
./handlers/compat/images.go:		Id             string            `json:"id"`
./handlers/compat/images.go:		Id             string            `json:"id"`
./handlers/types.go:	ID string `json:"id"`
./handlers/types.go:	ID string `json:"id"`
./handlers/types.go:	ID string `json:"id"`
./handlers/types.go:	ID             string `json:"id"`
./handlers/types.go:	ID string `json:"id"`
./handlers/types.go:	ID     string       `json:"id"`
./handlers/utils/containers.go:	ID string `json:"id"`

the only questionable ones i see for certain are the compat ones ... does the consumer side of those expect Id?

@@ -87,7 +87,7 @@ func ListContainers(w http.ResponseWriter, r *http.Request) {
utils.InternalServerError(w, err)
return
}
if _, found := r.URL.Query()["limit"]; found {
if _, found := r.URL.Query()["limit"]; found && query.Limit != -1 {
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't >= 0 be safer?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, then if user sends any negative number it will work.

@max-arnold
Copy link

To be honest, I haven’t looked how those ids are named in the Docker API spec. It is just a list of other possible occurrences to cross-check

@jwhonce
Copy link
Member

jwhonce commented Mar 19, 2020

Are these other occurrencies of lowercase id also should beId?

libpod/pkg/api % grep -r '"id"' .
./handlers/compat/images.go:		Id             string            `json:"id"`
./handlers/compat/images.go:		Id             string            `json:"id"`
./handlers/types.go:	ID string `json:"id"`
./handlers/types.go:	ID string `json:"id"`
./handlers/types.go:	ID string `json:"id"`
./handlers/types.go:	ID             string `json:"id"`
./handlers/types.go:	ID string `json:"id"`
./handlers/types.go:	ID     string       `json:"id"`
./handlers/utils/containers.go:	ID string `json:"id"`

the only questionable ones i see for certain are the compat ones ... does the consumer side of those expect Id?

If the /libpod tree doesn't follow the same naming WRT to common fields then bindings with case sensitive json keys won't port without a rewrite. A quick look this morning seems to be "id" for path queries and "Id" for responses.

@baude
Copy link
Member Author

baude commented Mar 19, 2020

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 19, 2020
@openshift-merge-robot openshift-merge-robot merged commit b43e249 into containers:master Mar 19, 2020
@baude baude deleted the compatfix branch May 7, 2020 13:10
@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.

APIv2 incompatibilities with docker-py
9 participants