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

implement header_upstream directive for #4, docs #8

Merged
merged 3 commits into from Sep 13, 2018

Conversation

erdii
Copy link
Collaborator

@erdii erdii commented Sep 10, 2018

No description provided.

@erdii erdii self-assigned this Sep 10, 2018
@coveralls
Copy link

coveralls commented Sep 10, 2018

Coverage Status

Coverage increased (+0.6%) to 88.435% when pulling 32b2bca on feature/header-upstream into 75731c3 on master.

@erdii
Copy link
Collaborator Author

erdii commented Sep 10, 2018

Hm.. for some strange reasons I cannot log into travis-ci.org and restart the failed pipeline. It failed with:

$ go get golang.org/x/tools/cmd/cover
package golang.org/x/tools/cmd/cover: unrecognized import path "golang.org/x/tools/cmd/cover" (https fetch: Get https://golang.org/x/tools/cmd/cover?go-get=1: net/http: TLS handshake timeout)
The command "go get golang.org/x/tools/cmd/cover" failed and exited with 1 during .

Since all other tests succeeded, I'll assume that a restart would indeed let the tests pass.

@coopernurse do you want to review the changes or can I merge?

README.md Outdated
```
header_upstream X-Forwarded-For {remote}
header_upstream X-Forwarded-Host {hostonly}
header_upstream X-Forwarded-Proto {scheme}
Copy link
Owner

Choose a reason for hiding this comment

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

Let's add a link to https://caddyserver.com/docs/placeholders in this doc section so folks know the valid set of placeholder variables.

It might also be nice to add an example that didn't use a placeholder, just to given an example of how that could be useful.

config.go Outdated
// uses caddy's integrated replacer for placeholder replacement (https://caddyserver.com/docs/placeholders)
for k, v := range c.UpstreamHeaders {
replInt := r.Context().Value(httpserver.ReplacerCtxKey)
replacer := replInt.(httpserver.Replacer)
Copy link
Owner

Choose a reason for hiding this comment

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

should replacer be defined outside the loop? I haven't read the docs on that type but I'm assuming we can safely call Replace() using the same instance inside the loop. Is that true?

Copy link
Owner

@coopernurse coopernurse left a comment

Choose a reason for hiding this comment

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

Just a couple of little things, but overall looks good!

@coopernurse
Copy link
Owner

I'm not sure what's up with the travis build status. I restarted both jobs but I'm not seeing status reported here for some reason. I think when the above issues are addressed we can just merge it and see if it builds correctly from master.

…older examples for header_upstream to README and linked caddy's placeholder docs
@erdii
Copy link
Collaborator Author

erdii commented Sep 10, 2018

Good points!

You're right, replacer should live outsive the loop in which it is used. Works exactly the same.
I also added a link to caddy's placeholder docs and a non-placeholder example.

Edit: but now the tests fail. I'm tired and will look into it tomorrow.

panic: interface conversion: interface is nil, not httpserver.Replacer [recovered]
	panic: interface conversion: interface is nil, not httpserver.Replacer

goroutine 16 [running]:
testing.tRunner.func1(0xc0001e7400)
	/usr/local/Cellar/go/1.11/libexec/src/testing/testing.go:792 +0x387
panic(0x16e6da0, 0xc0001ff800)
	/usr/local/Cellar/go/1.11/libexec/src/runtime/panic.go:513 +0x1b9
github.com/coopernurse/caddy-awslambda.(*Config).MaybeToInvokeInput(0xc000203ad0, 0xc0001e7500, 0x17a27ba, 0xd, 0x1)
	/Users/erdii/go/src/github.com/coopernurse/caddy-awslambda/config.go:161 +0x14f
github.com/coopernurse/caddy-awslambda.Handler.match(0x189a540, 0xc00020a4e0, 0xc00000e738, 0x1, 0x1, 0xc0001e7500, 0xc0001e4780, 0x40, 0x38, 0x1761720)
	/Users/erdii/go/src/github.com/coopernurse/caddy-awslambda/lambda.go:95 +0x11a
github.com/coopernurse/caddy-awslambda.Handler.ServeHTTP(0x189a540, 0xc00020a4e0, 0xc00000e738, 0x1, 0x1, 0x18a1c80, 0xc0001e4780, 0xc0001e7500, 0x0, 0x19afef8, ...)
	/Users/erdii/go/src/github.com/coopernurse/caddy-awslambda/lambda.go:28 +0x6d
github.com/coopernurse/caddy-awslambda.TestInvokeOK(0xc0001e7400)
	/Users/erdii/go/src/github.com/coopernurse/caddy-awslambda/lambda_test.go:28 +0x48b
testing.tRunner(0xc0001e7400, 0x17cbdc0)
	/usr/local/Cellar/go/1.11/libexec/src/testing/testing.go:827 +0xbf
created by testing.(*T).Run
	/usr/local/Cellar/go/1.11/libexec/src/testing/testing.go:878 +0x353
exit status 2
FAIL	github.com/coopernurse/caddy-awslambda	0.019s

@erdii
Copy link
Collaborator Author

erdii commented Sep 13, 2018

Now it should be ok.
lambda_test.go .TestInvokeOK had to use a request with replacer context, too

@erdii erdii dismissed coopernurse’s stale review September 13, 2018 10:39

Requested changes were incorporated

@erdii
Copy link
Collaborator Author

erdii commented Sep 13, 2018

@coopernurse How do we request changes to https://caddyserver.com/docs/http.awslambda ?

@coopernurse coopernurse merged commit e060a74 into master Sep 13, 2018
@coopernurse coopernurse deleted the feature/header-upstream branch September 13, 2018 13:57
@coopernurse
Copy link
Owner

@erdii Thanks for getting those changes in - it's merged now. Good question re: docs. I see mention of a developer portal now (fancy). I'll try to get in there and push an update.

@erdii
Copy link
Collaborator Author

erdii commented Sep 13, 2018

Nice!

@erdii erdii mentioned this pull request Sep 13, 2018
@coopernurse
Copy link
Owner

I was able to login and push an update. That kicked off a build somewhere and about 10 minutes later I got an email saying it worked.

However, this page still shows the old 2017 'last updated' date. https://caddyserver.com/docs/http.awslambda

So I'm not sure if there's anything else for us to do. Maybe the site docs only refresh daily? Let's keep an eye on it and see if it changes. But I think we're done.

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.

None yet

3 participants