Skip to content

Comments

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

Merged
vdemeester merged 1 commit intomoby:masterfrom
vdemeester:deprecated-copy-endpoint
Jun 7, 2016
Merged

Deprecate /containers/(id or name)/copy endpoint#22149
vdemeester merged 1 commit intomoby:masterfrom
vdemeester:deprecated-copy-endpoint

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 [email protected]

@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 <[email protected]>
@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 <[email protected]>
@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 <[email protected]>
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 <[email protected]>
(cherry picked from commit a34d804)
Signed-off-by: Cory Snider <[email protected]>
@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