-
Notifications
You must be signed in to change notification settings - Fork 25
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
Add support for multiple BACKEND by having them space-separated #40
Conversation
{{ if service $backend }} | ||
upstream {{ $backend }} { | ||
|
||
# loop for each backend |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you not mix tabs and spaces in this file please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, oops. Didn't even notice, relied on atom. :-)
I think we'd rather go with the approach in #37 where the end-user adds the config files for each backend as required. @jasonpincin is there anything else you'd like to add to that or should we merge it? |
I saw #37, was trying to get my head around it. Definitely like making this more extensible, but do we lose the example? It is a great way to jumpstart. |
@deitch the default configuration needs to be clear, obvious, and easily accessible for all users to understand. It's certainly possible to add support for a much broader variety of situations, but doing so at the cost of simplicity and clarity is a blow to those that look to this as an introductory example of how to use the pattern. It sounds like your goal is to use this in production. I haven't encountered a production use case yet that doesn't include application-specific configuration details, so I'd hesitate to make changes here that will significantly complicate the image for others without sufficiently addressing the production use-case to eliminate the need for a custom config (which would obviate the need for this PR). I assume you saw the pattern in https://github.com/autopilotpattern/wordpress, which extends the Nginx base image with a custom Nginx and ContainerPilot config? |
hey @misterbisson
Nope. Well, eventually, but not based on this. It was more for a POC, but would have been useful.
Certainly. I was looking to see if the base could be something more generic. |
(Oops, hit the button too soon) I do think that there is a lot of "yak shaving" in the wordpress example. See how much code is replicated between the base Mind you, #37 may help in that direction... |
Perhaps you should consider autopilotpattern/wordpress#19, which is what's required for a production implementation. The application-specific configuration significantly outweighs the shared configuration. I can't say that you're wrong, that the template can't be made to work for more situations, I don't think we can do that without increasing complexity and adding barriers to understanding. The cost of that is a couple extra files and separate build in a POC that I have to assume already includes at least one build. As you work through that and others, as these move beyond the POC stage, I'd very much like to hear from you about how your needs change. |
Small thing, but useful.
Sometimes you have a proxy in front of multiple services. The example nginx already has the backend proxied as
/<backend_service>
, so it is useful to have multiple. Just setBACKEND
to be space-separated multiple service names, and it creates multiple.E.g.
BACKEND="auth data wordpress"
will create three proxied URLs:/auth
/data
/wordpress