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.

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

Change custom headers separator #2509

Merged
merged 3 commits into from Dec 4, 2017
Merged

Change custom headers separator #2509

merged 3 commits into from Dec 4, 2017

Conversation

ldez
Copy link
Member

@ldez ldez commented Dec 1, 2017 •
edited

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
}

values := make(map[string]string)
for _, headers := range strings.Split(parts, "🧀") {
Copy link
Contributor

@dtomcej dtomcej Dec 1, 2017 •
edited by ldez

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
Member Author

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
LabelFrontendRequestHeader = LabelPrefix + "frontend.headers.customrequestheaders"
LabelFrontendResponseHeader = LabelPrefix + "frontend.headers.customresponseheaders"
LabelFrontendRequestHeader = LabelPrefix + "frontend.headers.customRequestHeaders"
LabelFrontendResponseHeader = LabelPrefix + "frontend.headers.customResponseHeaders"
Copy link
Contributor

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

For the record 😂

cheese

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

@dtomcej dtomcej dtomcej approved these changes

@traefiker traefiker traefiker approved these changes

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.

None yet
7 participants
@ldez @vito-c @emilevauge @juliens @mmatur @dtomcej @traefiker