search

mozilla / vinz-clortho

INACTIVE - http://mzl.la/ghe-archive - BrowserID Keymaster for LDAP enabled Identity Providers
16 stars 21 forks source link

`static_mount_path` configuration is not needed #32

Closed lloyd closed 11 years ago

lloyd commented 11 years ago

Whereby lloyd talks about killing mostly innocent code...

As far as I understand the rationale for static_mount_path, it's so that people deploying this server can proxy traffic to it based on path. They can root documents at a certain point /browserid and then they have an unambiguous route path.

We don't need this feature now nor in the forseeable future, and it makes the code more complex and harder to read. I'm weighing the tradeoffs here. I'm also wondering if we could reduce the number of lines of code it requires and the complexity by using postprocess.

My bias is to remove the config and the feature, and re-introduce it in a more minimal form once it's neccesary. But I'm talking about Killing Code here.

This would help me on my goal to get 90%+ test coverage before we go live :).

@ozten What do you think? Too aggressive? /cc @mostlygeek

ozten commented 11 years ago

If it's very hard to read, then okay. Otherwise, it's a feature that's already complete and works. It gives us flexibility in case our plans with IT change.

I'm also wondering if we could reduce the number of lines of code it requires and the complexity by using postprocess.

At first blush I'm against using postprocess. Too much magic.

mostlygeek commented 11 years ago

Less is more. If it's not a feature we're using at launch or foreseeable future then remove it.

FYI: I'm going to deploy the app behind nginx. I can add nginx confs to serve the static files directly. Also I'll be use the ELB to terminate SSL, so I might disable/remove that code as well.

lloyd commented 11 years ago

I can add nginx confs to serve the static files directly.

most or all of the static files are actually templates. let's talk through this together when you're ready to figure out what/whether nginx can/should serve static.

We may want to have nginx cache according to cache headers to get the benefits of it's crazy fast static file serving without any unexpected surprises?

ozten commented 11 years ago

FWIW: All static and dynamic files are "mounted" under this root.

Less is more. If it's not a feature we're using at launch or foreseeable future then remove it.

2:1 go ahead and nuke it.

I threw together a pull request before reading this.

lloyd commented 11 years ago

shit. @mostlygeek, now that ozten has mostly moved it out of the mainline code I'm less bullish. I'm wishy wash. I can't decide. I hereby appoint @mostlygeek to make the call.

mostlygeek commented 11 years ago

Then I say, take it out.