Skip to content

Deprecate /containers/(id or name)/copy endpoint #22149

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.

Sign up for GitHub

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

Jump to bottom
Merged
vdemeester merged 1 commit into from
Jun 7, 2016
Merged

Deprecate /containers/(id or name)/copy endpoint #22149

vdemeester merged 1 commit into from
Jun 7, 2016

Conversation

@vdemeester
Copy link
Member

@vdemeester vdemeester commented Apr 19, 2016 •
edited
Loading

This endpoint has been deprecated since 1.8. Return an error starting from this API version (1.24) in order to make sure it's not used for the next API version and so that we can remove it some times later 🐮.

This is probably the cleanest and simple way to handle that.

🐸

Signed-off-by: Vincent Demeester vincent@sbr.pm

@vdemeester vdemeester added this to the 1.12.0 milestone Apr 19, 2016
vdemeester
vdemeester reviewed Apr 19, 2016
View reviewed changes
api/server/router/container/copy.go Outdated
Copy link
Member Author

@vdemeester vdemeester Apr 19, 2016

Choose a reason for hiding this comment

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

Not sure this is the right error — as discussed a little bit with @thaJeztah, a simple HTTP 404 error should be fine.

@thaJeztah
Copy link
Member

thaJeztah commented Apr 19, 2016

In general, SGTM, we can discuss the implementation, and we need to have a note in the API changelog

@vdemeester vdemeester force-pushed the deprecated-copy-endpoint branch 2 times, most recently from 96a36d8 to 17e6819 Compare April 19, 2016 15:19
vdemeester
vdemeester reviewed Apr 19, 2016
View reviewed changes
api/server/router/container/copy.go Outdated
Copy link
Member Author

@vdemeester vdemeester Apr 19, 2016

Choose a reason for hiding this comment

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

I probably should add a comment here (instead of in api/server/router/container/container.go

Copy link
Member

@thaJeztah thaJeztah May 12, 2016

Choose a reason for hiding this comment

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

👍

@vdemeester vdemeester force-pushed the deprecated-copy-endpoint branch 2 times, most recently from 211b278 to e3d9b60 Compare April 19, 2016 15:40
@thaJeztah
Copy link
Member

thaJeztah commented May 12, 2016

moved this to code review; can you update the comment and documentation?

@vdemeester vdemeester force-pushed the deprecated-copy-endpoint branch from e3d9b60 to 62fc0ce Compare May 26, 2016 15:07
@vdemeester
Copy link
Member Author

vdemeester commented May 26, 2016

Updated 👼

@vdemeester vdemeester force-pushed the deprecated-copy-endpoint branch from 62fc0ce to 52f1d53 Compare May 26, 2016 15:50
icecrime
icecrime reviewed May 27, 2016
View reviewed changes
integration-cli/docker_api_containers_test.go Outdated
Copy link
Contributor

@icecrime icecrime May 27, 2016

Choose a reason for hiding this comment

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

/vcontainers/?

Copy link
Member Author

@vdemeester vdemeester May 27, 2016

Choose a reason for hiding this comment

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

damn me 😭

@vdemeester vdemeester force-pushed the deprecated-copy-endpoint branch from 52f1d53 to 1cbf51a Compare May 28, 2016 08:46
@thaJeztah
Copy link
Member

thaJeztah commented Jun 1, 2016

LGTM, don't forget to remove it from the API docs; https://github.com/docker/docker/blob/master/docs/reference/api/docker_remote_api_v1.24.md#copy-files-or-folders-from-a-container

@vdemeester vdemeester force-pushed the deprecated-copy-endpoint branch from 1cbf51a to 6b01d44 Compare June 1, 2016 14:48
@vdemeester
Copy link
Member Author

vdemeester commented Jun 1, 2016

@thaJeztah ahh good point 😊 Updated 👼

@thaJeztah
Copy link
Member

thaJeztah commented Jun 1, 2016

LGTM (code and docs)

@thaJeztah
Copy link
Member

thaJeztah commented Jun 3, 2016

ping @vdemeester sorry, this needs a rebase 😢

@vdemeester vdemeester force-pushed the deprecated-copy-endpoint branch 2 times, most recently from 2e46a3e to e452ebc Compare June 3, 2016 12:36
@thaJeztah
Copy link
Member

thaJeztah commented Jun 3, 2016

oh! actually, can you add an entry to the "deprecated features" page? This endpoint was deprecated before we had that document, so I think we should add a mention there.

Also just discussing with @justincormack that we should have a deprecation policy for old API version (how many versions should we support?) otherwise removing these endpoints won't bring us much, because we still have to maintain it if we keep the old API versions

@vdemeester
Copy link
Member Author

vdemeester commented Jun 3, 2016

@thaJeztah ok 😝 and completely agree with the depreciation policy 😇

@vdemeester
Deprecate /containers/(id or name)/copy endpoint
4283289 
This endpoint has been deprecated since 1.8. Return an error starting
from this API version (1.24) in order to make sure it's not used for the
next API version and so that we can remove it some times later.

Signed-off-by: Vincent Demeester <vincent@sbr.pm>
@vdemeester vdemeester force-pushed the deprecated-copy-endpoint branch from e452ebc to 4283289 Compare June 3, 2016 17:38
@thaJeztah thaJeztah mentioned this pull request Jun 3, 2016
Closed
@thaJeztah
Copy link
Member

thaJeztah commented Jun 5, 2016

ping @justincormack @calavera PTAL

@runcom
Copy link
Member

runcom commented Jun 7, 2016

LGTM

@vdemeester
Copy link
Member Author

vdemeester commented Jun 7, 2016

Author docs LGTM 😝
Oh I can do that right ? 👼

@vdemeester vdemeester merged commit 148d2b8 into moby:master Jun 7, 2016
@vdemeester vdemeester deleted the deprecated-copy-endpoint branch June 7, 2016 10:50
@thaJeztah thaJeztah mentioned this pull request Jan 10, 2017
Closed
rvolosatovs pushed a commit to rvolosatovs/moby that referenced this pull request Jul 26, 2021
integration: remove deprecated endpoint test
a34d804 
`/containers/<name>/copy` endpoint was deprecated in 1.8 and errors
since 1.12. See moby#22149 for more info.

Signed-off-by: Roman Volosatovs <roman.volosatovs@docker.com>
@rvolosatovs rvolosatovs mentioned this pull request Jul 26, 2021
Merged
docker-jenkins pushed a commit to docker-archive/docker-ce that referenced this pull request Jul 27, 2021
integration: remove deprecated endpoint test
c80e6d8 
`/containers/<name>/copy` endpoint was deprecated in 1.8 and errors
since 1.12. See moby/moby#22149 for more info.

Signed-off-by: Roman Volosatovs <roman.volosatovs@docker.com>
Upstream-commit: a34d804572882e2251de7769efd075886c76ea10
Component: engine
corhere pushed a commit to corhere/moby that referenced this pull request Oct 20, 2023
@corhere
integration: remove deprecated endpoint test
526d214 
`/containers/<name>/copy` endpoint was deprecated in 1.8 and errors
since 1.12. See moby#22149 for more info.

Signed-off-by: Roman Volosatovs <roman.volosatovs@docker.com>
(cherry picked from commit a34d804)
Signed-off-by: Cory Snider <csnider@mirantis.com>
@thaJeztah thaJeztah mentioned this pull request Jan 22, 2024
Merged
3 tasks
@thaJeztah thaJeztah mentioned this pull request Jun 8, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

impact/api impact/changelog impact/deprecation status/3-docs-review

Projects

None yet

Milestone

1.12.0

Development

Successfully merging this pull request may close these issues.

5 participants

@vdemeester @thaJeztah @runcom @icecrime @GordonTheTurtle