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

Change error message for compatibility with docker #12318

Merged
merged 1 commit into from
Nov 19, 2021

Conversation

mscherer
Copy link
Contributor

Fixes #12315

Signed-off-by: Michael Scherer [email protected]

[ NO NEW TESTS NEEDED ]

What this PR does / why we need it:

How to verify it

Which issue(s) this PR fixes:

Special notes for your reviewer:

@Luap99
Copy link
Member

Luap99 commented Nov 16, 2021

I do not think this is the right approach. utils.Error() is used for all error in the API. We should not change this. This will also impact the libpod api. To fix this only the compat api has to return the correct error struct. I assume this will affect all docker compat endpoints not just this one.

@mscherer
Copy link
Contributor Author

Indeed. So I tested that PR, and it seems to fix my initial problem. Not sure if that fix all instance of the same issue.

@mscherer
Copy link
Contributor Author

(when I say "test that PR", I mean, the latest iteration, where the error is wrapped, and only for the compat API, sorry if that's not clear)

@rhatdan
Copy link
Member

rhatdan commented Nov 16, 2021

@Luap99 @mheon @jwhonce utils.Error currently takes in an APIMessage and then does nothing with it. Should we change the API to drop this or should we wrap the error message with the API Message?

@jwhonce
Copy link
Member

jwhonce commented Nov 18, 2021

@rhatdan The initial implementation had Cause as the error.Error() text while Message was a human readable message for presentation. That got lost over time. Now we have Cause == error.Cause().Error() and Message == error.Error(). I am okay with removing the parameter apiMessage as it is meaningless and unused at this time.

@rhatdan
Copy link
Member

rhatdan commented Nov 18, 2021

@jwhonce Are you ok with merging this PR?

@jwhonce
Copy link
Member

jwhonce commented Nov 18, 2021

/merge

@rhatdan
Copy link
Member

rhatdan commented Nov 19, 2021

/approve
/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 19, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Nov 19, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mscherer, rhatdan

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 openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 19, 2021
@openshift-merge-robot openshift-merge-robot merged commit 87a7800 into containers:main Nov 19, 2021
@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 22, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 22, 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.

5 participants