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

Dockerd: enable CORS when only --api-cors-header is used #32174

Merged
merged 1 commit into from
Mar 30, 2017
Merged

Dockerd: enable CORS when only --api-cors-header is used #32174

merged 1 commit into from
Mar 30, 2017

Conversation

KarthikNayak
Copy link
Contributor

@KarthikNayak KarthikNayak commented Mar 28, 2017

Even though the flag --api-enable-cors is deprecated in favor of
--api-cors-header. Using only --api-cors-header does not enable
CORS.

Make changes to 'cmd/dockerd/daemon.go' to enable CORS if either of
the above flags is set.

Signed-off-by: Karthik Nayak Karthik.188@gmail.com

This PR closes the issue #32113

- What I did

Fixed the issue mentioned above.

- How I did it

Allowed CORS to be enabled even if --api-enable-cors is not used but --api-cors-header is set, by changing the if condition in cmd/dockerd/daemon.go.

- How to verify it

Check issue for steps to reproduce and test.

- Description for the changelog

Fix an issue where CORS (--api-cors-header) was ignored if --api-enable-cors was not set

- A picture of a cute animal (not mandatory but encouraged)

10e9721ebeb269bdd1c434f08572d81d

Img Credit

Even though the flag `--api-enable-cors` is deprecated in favor of
`--api-cors-header`. Using only `--api-cors-header` does not enable
CORS.

Make changes to 'cmd/dockerd/daemon.go' to enable cors if either of
the above flags is set.

Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM

I looked through the code, and I see this flag was deprecated a long time ago (Docker 1.6); #10586, at which point there was no "official" deprecation policy, so it's not mentioned in https://github.com/docker/docker/blob/master/docs/deprecated.md.

Would you be interested in doing a follow-up to add it to the "deprecated.md" file? We usually keep a 3 "stable" release deprecation cycle, but in this case, I think we should be ok with actually removing it in docker 17.09

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM

I looked through the code, and I see this flag was deprecated a long time ago (Docker 1.6); #10586, at which point there was no "official" deprecation policy, so it's not mentioned in https://github.com/docker/docker/blob/master/docs/deprecated.md.

Would you be interested in doing a follow-up to add it to the "deprecated.md" file? We usually keep a 3 "stable" release deprecation cycle, but in this case, I think we should be ok with actually removing it in docker 17.09

@thaJeztah
Copy link
Member

oh, sorry for the double post, I got an "angry unicorn" at GitHub

@KarthikNayak
Copy link
Contributor Author

Sure I'll do that, Should I push to the same branch (That would show up in this PR) or a new branch/PR?

@thaJeztah
Copy link
Member

the deprecation is best done in a separate PR to not block this one; it may need a short discussion (of everyone agrees on the deprecation cycle - perhaps they're all ok with removing it directly)

@ehazlett
Copy link
Contributor

LGTM 👍

@ehazlett ehazlett merged commit 59aed5a into moby:master Mar 30, 2017
@GordonTheTurtle GordonTheTurtle added this to the 17.05.0 milestone Mar 30, 2017
@thaJeztah
Copy link
Member

thaJeztah commented Mar 30, 2017

whoop, thanks @KarthikNayak 🎉!

screen shot 2017-03-30 at 17 26 29

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants