Issue 29427603: Noissue - Configure discourse containers to use smtp on the host system
|
Created:
May 2, 2017, 9:23 p.m. by f.nicolaisen Modified:
July 10, 2017, 11:38 a.m. Reviewers:
mathias CC:
f.lopez Base URL:
https://hg1/infrastructure Visibility:
Public. |
DescriptionNoissue - Configure discourse containers to use smtp on the host system
Note that this requires an smtp service on the host system that allows connections from docker containers.
See comment in the ticket below to see how this can be configured in the host's postfix.
See http://hub.eyeo.com/issues/71#note-14
Patch Set 1 #
Total comments: 6
Patch Set 2 : use host ip instead of undef #
MessagesTotal messages: 14
The comment applies for both yaml files, eyeoforum1 and abpforum1 https://codereview.adblockplus.org/29427603/diff/29427604/modules/private-stu... File modules/private-stub/hiera/hosts/eyeoforum1.yaml (right): https://codereview.adblockplus.org/29427603/diff/29427604/modules/private-stu... modules/private-stub/hiera/hosts/eyeoforum1.yaml:13: smtp_host: "172.17.0.1" This seems arbitrary to me, any particular reason for this IP?
https://codereview.adblockplus.org/29427603/diff/29427604/modules/private-stu... File modules/private-stub/hiera/hosts/eyeoforum1.yaml (right): https://codereview.adblockplus.org/29427603/diff/29427604/modules/private-stu... modules/private-stub/hiera/hosts/eyeoforum1.yaml:13: smtp_host: "172.17.0.1" On 2017/05/04 17:10:10, f.lopez wrote: > This seems arbitrary to me, any particular reason for this IP? Yes, it's the typical docker host IP address. I noted in the issue, but should probably include in the commit message: > Note that the above may break at any time. There is no good way in docker to get the ip of the host (by design). The solution is to use the company smtp_host, when we get one.
https://codereview.adblockplus.org/29427603/diff/29427604/modules/discourse_d... File modules/discourse_docker/manifests/init.pp (right): https://codereview.adblockplus.org/29427603/diff/29427604/modules/discourse_d... modules/discourse_docker/manifests/init.pp:55: $smtp_host = hiera('discourse_docker::smtp_host', undef), Since the template does not place any conditional around the use of this parameter it should default to something more useful than undef.
https://codereview.adblockplus.org/29427603/diff/29427604/modules/discourse_d... File modules/discourse_docker/manifests/init.pp (right): https://codereview.adblockplus.org/29427603/diff/29427604/modules/discourse_d... modules/discourse_docker/manifests/init.pp:55: $smtp_host = hiera('discourse_docker::smtp_host', undef), On 2017/07/07 19:45:43, mathias wrote: > Since the template does not place any conditional around the use of this > parameter it should default to something more useful than undef. We could use 172.17.0.1 as configured in the hiera examples. This will work in current vanilla docker containers, but it's an anti-pattern. Alternatively, I can raise an error if the smtp_host is not explicitly provided. What do you think?
https://codereview.adblockplus.org/29427603/diff/29427604/modules/discourse_d... File modules/discourse_docker/manifests/init.pp (right): https://codereview.adblockplus.org/29427603/diff/29427604/modules/discourse_d... modules/discourse_docker/manifests/init.pp:55: $smtp_host = hiera('discourse_docker::smtp_host', undef), On 2017/07/10 08:29:08, f.nicolaisen wrote: > On 2017/07/07 19:45:43, mathias wrote: > > Since the template does not place any conditional around the use of this > > parameter it should default to something more useful than undef. > > We could use 172.17.0.1 as configured in the hiera examples. This will work in > current vanilla docker containers, but it's an anti-pattern. > > Alternatively, I can raise an error if the smtp_host is not explicitly provided. > > > What do you think? Why not use the formerly hard-coded DISCOURSE_SMTP_ADDRESS value: localhost?
On 2017/07/10 08:32:43, mathias wrote: > https://codereview.adblockplus.org/29427603/diff/29427604/modules/discourse_d... > File modules/discourse_docker/manifests/init.pp (right): > > https://codereview.adblockplus.org/29427603/diff/29427604/modules/discourse_d... > modules/discourse_docker/manifests/init.pp:55: $smtp_host = > hiera('discourse_docker::smtp_host', undef), > On 2017/07/10 08:29:08, f.nicolaisen wrote: > > On 2017/07/07 19:45:43, mathias wrote: > > > Since the template does not place any conditional around the use of this > > > parameter it should default to something more useful than undef. > > > > We could use 172.17.0.1 as configured in the hiera examples. This will work in > > current vanilla docker containers, but it's an anti-pattern. > > > > Alternatively, I can raise an error if the smtp_host is not explicitly > provided. > > > > > > What do you think? > > Why not use the formerly hard-coded DISCOURSE_SMTP_ADDRESS value: localhost? There is no smtp service on localhost. The container has no postfix installed.
https://codereview.adblockplus.org/29427603/diff/29427604/modules/discourse_d... File modules/discourse_docker/manifests/init.pp (right): https://codereview.adblockplus.org/29427603/diff/29427604/modules/discourse_d... modules/discourse_docker/manifests/init.pp:55: $smtp_host = hiera('discourse_docker::smtp_host', undef), On 2017/07/10 08:32:42, mathias wrote: > On 2017/07/10 08:29:08, f.nicolaisen wrote: > > On 2017/07/07 19:45:43, mathias wrote: > > > Since the template does not place any conditional around the use of this > > > parameter it should default to something more useful than undef. > > > > We could use 172.17.0.1 as configured in the hiera examples. This will work in > > current vanilla docker containers, but it's an anti-pattern. > > > > Alternatively, I can raise an error if the smtp_host is not explicitly > provided. > > > > > > What do you think? > > Why not use the formerly hard-coded DISCOURSE_SMTP_ADDRESS value: localhost? There is no smtp service on localhost. The container has no postfix installed.
On 2017/07/10 08:39:45, f.nicolaisen wrote: > https://codereview.adblockplus.org/29427603/diff/29427604/modules/discourse_d... > File modules/discourse_docker/manifests/init.pp (right): > > https://codereview.adblockplus.org/29427603/diff/29427604/modules/discourse_d... > modules/discourse_docker/manifests/init.pp:55: $smtp_host = > hiera('discourse_docker::smtp_host', undef), > On 2017/07/10 08:32:42, mathias wrote: > > On 2017/07/10 08:29:08, f.nicolaisen wrote: > > > On 2017/07/07 19:45:43, mathias wrote: > > > > Since the template does not place any conditional around the use of this > > > > parameter it should default to something more useful than undef. > > > > > > We could use 172.17.0.1 as configured in the hiera examples. This will work > in > > > current vanilla docker containers, but it's an anti-pattern. > > > > > > Alternatively, I can raise an error if the smtp_host is not explicitly > > provided. > > > > > > > > > What do you think? > > > > Why not use the formerly hard-coded DISCOURSE_SMTP_ADDRESS value: localhost? > > There is no smtp service on localhost. The container has no postfix installed. Sounds like a good default to me. At least the configuration won't break in (dev/test) setups where mail isn't part of the plan. Alternatively you could just not write any mail configuration if the parameter is missing, or configure a manual check and error. Not sure which one is best, not enough insight for that, but just silently writing an invalid configuration doesn't sound reasonable.
On 2017/07/10 09:18:24, mathias wrote: > On 2017/07/10 08:39:45, f.nicolaisen wrote: > > > https://codereview.adblockplus.org/29427603/diff/29427604/modules/discourse_d... > > File modules/discourse_docker/manifests/init.pp (right): > > > > > https://codereview.adblockplus.org/29427603/diff/29427604/modules/discourse_d... > > modules/discourse_docker/manifests/init.pp:55: $smtp_host = > > hiera('discourse_docker::smtp_host', undef), > > On 2017/07/10 08:32:42, mathias wrote: > > > On 2017/07/10 08:29:08, f.nicolaisen wrote: > > > > On 2017/07/07 19:45:43, mathias wrote: > > > > > Since the template does not place any conditional around the use of this > > > > > parameter it should default to something more useful than undef. > > > > > > > > We could use 172.17.0.1 as configured in the hiera examples. This will > work > > in > > > > current vanilla docker containers, but it's an anti-pattern. > > > > > > > > Alternatively, I can raise an error if the smtp_host is not explicitly > > > provided. > > > > > > > > > > > > What do you think? > > > > > > Why not use the formerly hard-coded DISCOURSE_SMTP_ADDRESS value: localhost? > > > > There is no smtp service on localhost. The container has no postfix installed. > > Sounds like a good default to me. Which part? > At least the configuration won't break in > (dev/test) setups where mail isn't part of the plan. Alternatively you could > just not write any mail configuration if the parameter is missing, or configure > a manual check and error. Not sure which one is best, not enough insight for > that, but just silently writing an invalid configuration doesn't sound > reasonable. OK, writing localhost as it is now would be an invalid configuration. Discourse is very dependent on email working [1]. It *can* be set up locally without it, but it is hacky. I'll go for the manual check and error next time I work on this, unless something else is suggested. That also feels future compatible with us having an external mail service available. [1] https://github.com/discourse/discourse/blob/master/docs/INSTALL-cloud.md (search for email)
On 2017/07/10 09:43:18, f.nicolaisen wrote: > On 2017/07/10 09:18:24, mathias wrote: > > On 2017/07/10 08:39:45, f.nicolaisen wrote: > > > > > > https://codereview.adblockplus.org/29427603/diff/29427604/modules/discourse_d... > > > File modules/discourse_docker/manifests/init.pp (right): > > > > > > > > > https://codereview.adblockplus.org/29427603/diff/29427604/modules/discourse_d... > > > modules/discourse_docker/manifests/init.pp:55: $smtp_host = > > > hiera('discourse_docker::smtp_host', undef), > > > On 2017/07/10 08:32:42, mathias wrote: > > > > On 2017/07/10 08:29:08, f.nicolaisen wrote: > > > > > On 2017/07/07 19:45:43, mathias wrote: > > > > > > Since the template does not place any conditional around the use of > this > > > > > > parameter it should default to something more useful than undef. > > > > > > > > > > We could use 172.17.0.1 as configured in the hiera examples. This will > > work > > > in > > > > > current vanilla docker containers, but it's an anti-pattern. > > > > > > > > > > Alternatively, I can raise an error if the smtp_host is not explicitly > > > > provided. > > > > > > > > > > > > > > > What do you think? > > > > > > > > Why not use the formerly hard-coded DISCOURSE_SMTP_ADDRESS value: > localhost? > > > > > > There is no smtp service on localhost. The container has no postfix > installed. > > > > Sounds like a good default to me. > > Which part? > > > At least the configuration won't break in > > (dev/test) setups where mail isn't part of the plan. Alternatively you could > > just not write any mail configuration if the parameter is missing, or > configure > > a manual check and error. Not sure which one is best, not enough insight for > > that, but just silently writing an invalid configuration doesn't sound > > reasonable. > > OK, writing localhost as it is now would be an invalid configuration. > > Discourse is very dependent on email working [1]. It *can* be set up locally > without it, but it is hacky. > > I'll go for the manual check and error next time I work on this, unless > something else is suggested. That also feels future compatible with us having an > external mail service available. > > [1] https://github.com/discourse/discourse/blob/master/docs/INSTALL-cloud.md > (search for email) How can one test this e-mail stuff in development anyway? Shouldn't whatever value is necessary for that be the default?
On 2017/07/10 09:48:15, mathias wrote: > How can one test this e-mail stuff in development anyway? Shouldn't whatever > value is necessary for that be the default? In development the host system (172.17.0.1) has postfix installed, so that can be used for testing (i.e. checking mailboxes or logs on the host system).
On 2017/07/10 09:52:49, f.nicolaisen wrote: > On 2017/07/10 09:48:15, mathias wrote: > > How can one test this e-mail stuff in development anyway? Shouldn't whatever > > value is necessary for that be the default? > > In development the host system (172.17.0.1) has postfix installed, so that can > be used for testing (i.e. checking mailboxes or logs on the host system). Ok, thank you for the info. I think that and the class being named *_docker makes this a valid option, better than undef at least.
On 2017/07/10 10:05:06, mathias wrote: > On 2017/07/10 09:52:49, f.nicolaisen wrote: > > On 2017/07/10 09:48:15, mathias wrote: > > > How can one test this e-mail stuff in development anyway? Shouldn't whatever > > > value is necessary for that be the default? > > > > In development the host system (172.17.0.1) has postfix installed, so that can > > be used for testing (i.e. checking mailboxes or logs on the host system). > > Ok, thank you for the info. I think that and the class being named *_docker > makes this a valid option, better than undef at least. OK, PS 2 changes undef to the host IP (and replaces some double quotes with single). 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
