Skip to content

Change custom headers separator #2509

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
traefiker merged 3 commits into from
Dec 4, 2017
Merged

Change custom headers separator #2509

traefiker merged 3 commits into from
Dec 4, 2017

Conversation

@ldez
Copy link
Contributor

@ldez ldez commented Dec 1, 2017 •
edited
Loading

What does this PR do?

  • Change custom headers separator. , -> ||
  • Rename the Docker labels

Motivation

Fixes #2508, #2517

More

  • Added/updated documentation

@ldez ldez requested review from a team as code owners December 1, 2017 13:07
@traefiker traefiker added this to the 1.5 milestone Dec 1, 2017
dtomcej
dtomcej suggested changes Dec 1, 2017
View reviewed changes
provider/docker/labels.go Outdated
Copy link
Contributor

@dtomcej dtomcej Dec 1, 2017 •
edited by ldez
Loading

Choose a reason for hiding this comment

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

Since we are splitting the headers in the label on this emoji, are we going to get users to separate custom headers etc on 🧀 ?

X-Custom-Header:foo🧀X-Other-Header:bar?

Copy link
Contributor Author

@ldez ldez Dec 1, 2017

Choose a reason for hiding this comment

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

It's a joke! 😂

types/common_label.go Outdated
Copy link
Contributor

@dtomcej dtomcej Dec 1, 2017

Choose a reason for hiding this comment

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

👍

dtomcej
dtomcej previously approved these changes Dec 1, 2017
View reviewed changes
@vito-c
Copy link

vito-c commented Dec 1, 2017

are the custom headers supposed to show up in the admin? I'm testing this PR with ; as the delimiter right now but I don't see them in the admin.

@vito-c
Copy link

vito-c commented Dec 1, 2017 

          "customRequestHeaders": {
            "Access-Control-Allow-Headers": "accept,origin,xsrf-token,x-requested-with,x-rally-auth-token,x-rally-authtoken,x-rally-verification-code,x-rally-locale,x-rally-userId,x-rally-session-token, x-rally-sessiontoken, server-event-uuid, content-type, x-abuse-info",
            "Access-Control-Allow-Methods": "GET,POST,OPTIONS,DELETE,PUT",

from the api request

@emilevauge
Copy link
Member

emilevauge commented Dec 1, 2017 •
edited by ldez
Loading

For the record 😂

cheese

@ldez ldez force-pushed the fix/headers-parser branch from dcb660d to 57b6c28 Compare December 2, 2017 03:03
mmatur
mmatur approved these changes Dec 2, 2017
View reviewed changes
Copy link
Member

@mmatur mmatur left a comment

Choose a reason for hiding this comment

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

LGTM - Nice job @ldez 👏

dtomcej
dtomcej approved these changes Dec 2, 2017
View reviewed changes
Copy link
Contributor

@dtomcej dtomcej left a comment

Choose a reason for hiding this comment

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

Real Review:
LGTM!
:shipit:

@ldez ldez force-pushed the fix/headers-parser branch 2 times, most recently from fb24b8d to b4ae7b4 Compare December 4, 2017 09:38
juliens
juliens approved these changes Dec 4, 2017
View reviewed changes
Copy link
Member

@juliens juliens left a comment

Choose a reason for hiding this comment

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

LGTM

@traefiker traefiker merged commit 3a99c86 into traefik:v1.5 Dec 4, 2017
@traefiker traefiker mentioned this pull request Dec 4, 2017
Closed
@ldez ldez deleted the fix/headers-parser branch December 4, 2017 12:31
This was referenced Dec 4, 2017
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@juliens juliens juliens approved these changes

@mmatur mmatur mmatur approved these changes

@traefiker traefiker traefiker approved these changes

+1 more reviewer

@dtomcej dtomcej dtomcej approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area/provider/docker area/provider/k8s/ingress kind/bug/fix a bug fix size/S

Projects

None yet

Milestone

1.5

Development

Successfully merging this pull request may close these issues.

7 participants

@ldez @vito-c @emilevauge @juliens @mmatur @dtomcej @traefiker