Skip to content
This repository has been archived by the owner on Jan 24, 2019. It is now read-only.

Add support for serving static files from a directory #142

Merged
merged 1 commit into from
Sep 24, 2015
Merged

Add support for serving static files from a directory #142

merged 1 commit into from
Sep 24, 2015

Conversation

Tenzer
Copy link
Contributor

@Tenzer Tenzer commented Sep 23, 2015

As discussed in #141, this adds the possibility of hosting static files with oauth2_proxy.

The path should be provided as a file:/// url with the full operating system path, ie. file:///var/www/static/.

An alias to where the directory is available as can be specified by appending a fragment (ie. "#/static/") at the end of the URL, otherwise the full file path has to be entered in the URL when requesting the files.

Let me know if you have any comments on this. If not then I'll try to get it added to the README file.

setProxyUpstreamHostHeader(proxy, u)
} else {
setProxyDirector(proxy)
if u.Scheme == "http" {
Copy link
Member

Choose a reason for hiding this comment

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

can you make this a case statement (Go convention is to prefer case over if/else in many cases. pun intended =))

switch u.Scheme {
case "http", "https":
   ...
case "file":
   ...
default:
   panic(fmt.Sprintf("unknown upstream protocol %s", u.Scheme))
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I have switched to a case statement (I can make puns as well ;)) in a4feea9. The reason why I did the if/else is probably due to I normally work in Python, which doesn't have a switch/case statement.

@jehiah
Copy link
Member

jehiah commented Sep 23, 2015

@Tenzer this looks like a great first path. I think using the fragment as the path is clever.

I need to think a little about what that means for typical configurations, and if there should be a way for us to support relative filesystem paths. In the mean time take a pass at README updates and I can always follow up with a future update for relative paths if needed.

@Tenzer
Copy link
Contributor Author

Tenzer commented Sep 23, 2015

Hmm, tests are failing now due to the check of the scheme for the provided upstreams.

I don't think there's an official way of supporting relative paths in file:// URLs. You would instead probably have to make a convention where file:///./in the beginning of upstream configuration would be replaced with the current folder. I doubt that would give any problems, since I don't think you can make a folder named . in any Unix system.

I'll add some details to the README file.

@Tenzer
Copy link
Contributor Author

Tenzer commented Sep 23, 2015

I have added a section to the README file now which describes a bit more in general how to configure upstreams as #12 also requested that. There may be changes that has to be made to the text, but we at least have something to base corrections upon now.

@Tenzer
Copy link
Contributor Author

Tenzer commented Sep 24, 2015

I think the easiest way to fix the failing tests, is simply to remove the lines inside oauthproxy_test.go which add an upstream of "unused" as NewOauthProxy() in oauthproxy.go then wont iterate over the upstream and complain about the scheme being off. From what I can see, adding a upstream of "unused" doesn't add anything and there's no checking in the tests for if a upstream is provided at all.

@jehiah
Copy link
Member

jehiah commented Sep 24, 2015

@Tenzer that or updating the value to http://unused/. Once the tests pass go ahead and squash this down to a single commit and we'll land this PR. The docs look good.

The path should be provided as a file:// url with the full operating system path.
An alias to where the directory is available as can be specified by appending
a fragment (ie. "#/static/") at the end of the URL.
@Tenzer
Copy link
Contributor Author

Tenzer commented Sep 24, 2015

I opted for removing the lines since they aren't part of what is being tested. In case a later addition would check if the upstream is available before it get added the tests would fail again.

The commits has been squashed and the tests has passed. Thanks for the guidance on adding this!

jehiah added a commit that referenced this pull request Sep 24, 2015
Add support for serving static files from a directory
@jehiah jehiah merged commit 3ed828e into bitly:master Sep 24, 2015
@jehiah
Copy link
Member

jehiah commented Sep 24, 2015

⭐ 🚀 🎉 awesome work!

@jehiah jehiah mentioned this pull request Sep 24, 2015
@Tenzer Tenzer deleted the static-file-server branch September 24, 2015 13:50
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

Successfully merging this pull request may close these issues.

2 participants