Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(367)

Issue 5766400081657856: #760 - Make our ssh configuration used automatically for all servers (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
5 years, 4 months ago by mathias
Modified:
5 years, 4 months ago
Reviewers:
Wladimir Palant
Visibility:
Public.

Description

See https://issues.adblockplus.org/ticket/760

Patch Set 1 #

Total comments: 10

Patch Set 2 : #760 - Make our ssh configuration used automatically for all servers #

Patch Set 3 : #760 - Make our ssh configuration used automatically for all servers #

Unified diffs Side-by-side diffs Delta from patch set Stats (+37 lines, -20 lines) Patch
M modules/base/manifests/init.pp View 1 chunk +1 line, -1 line 0 comments Download
M modules/filtermaster/manifests/init.pp View 1 2 1 chunk +19 lines, -10 lines 0 comments Download
M modules/ssh/files/sshd_config View 1 2 1 chunk +0 lines, -1 line 0 comments Download
M modules/ssh/manifests/init.pp View 1 2 1 chunk +10 lines, -5 lines 0 comments Download
M modules/statsclient/manifests/init.pp View 1 2 1 chunk +7 lines, -3 lines 0 comments Download

Messages

Total messages: 5
Wladimir Palant
http://codereview.adblockplus.org/5766400081657856/diff/5629499534213120/modules/filtermaster/manifests/init.pp File modules/filtermaster/manifests/init.pp (right): http://codereview.adblockplus.org/5766400081657856/diff/5629499534213120/modules/filtermaster/manifests/init.pp#newcode8 modules/filtermaster/manifests/init.pp:8: order => '02', This doesn't need to be sequential, ...
5 years, 4 months ago (2014-07-15 11:56:47 UTC) #1
mathias
http://codereview.adblockplus.org/5766400081657856/diff/5629499534213120/modules/filtermaster/manifests/init.pp File modules/filtermaster/manifests/init.pp (right): http://codereview.adblockplus.org/5766400081657856/diff/5629499534213120/modules/filtermaster/manifests/init.pp#newcode17 modules/filtermaster/manifests/init.pp:17: content => ' On 2014/07/15 11:56:47, Wladimir Palant wrote: ...
5 years, 4 months ago (2014-07-15 12:03:12 UTC) #2
mathias
See https://issues.adblockplus.org/ticket/760
5 years, 4 months ago (2014-07-15 12:59:35 UTC) #3
mathias
5 years, 4 months ago (2014-07-15 13:21:54 UTC) #4
Wladimir Palant
5 years, 4 months ago (2014-07-15 14:18:35 UTC) #5
LGTM

http://codereview.adblockplus.org/5766400081657856/diff/5629499534213120/modu...
File modules/filtermaster/manifests/init.pp (right):

http://codereview.adblockplus.org/5766400081657856/diff/5629499534213120/modu...
modules/filtermaster/manifests/init.pp:17: content => '
On 2014/07/15 12:03:12, matze wrote:
> Either the non-block settings (see your comment above) or block settings
should
> become sequential. I dislike to do that for both types. Which do you prefer?
(I
> prefer the former option.)

I don't really understand your comment. My understanding of sshd_config is that
ordering matters - Match blocks always have to be placed at the end of the file.
It might be that omitting the order parameter will also place the fragment at
the end but the documentation doesn't really say what will happen in that
scenario so I would consider the behavior as undefined. Hence my suggestion to
always use '99' for blocks.

Using '50' instead of '02' was meant simply in case we need to add more
fragments to the generic part. We can have as convention that anything below
'50' is generic whereas '50' and above marks custom configuration.

http://codereview.adblockplus.org/5766400081657856/diff/5629499534213120/modu...
File modules/ssh/templates/sshd_config.erb (right):

http://codereview.adblockplus.org/5766400081657856/diff/5629499534213120/modu...
modules/ssh/templates/sshd_config.erb:90: UsePAM yes
On 2014/07/15 12:03:12, matze wrote:
> Well, I could imagine performing some future global setup by passing class
> parameters through to the template

Well, I cannot - that fragment is being created via base module, we don't have
any server-specific parameters at this point so I don't see any reason why we
would use templates. Either way, our general approach is not to keep things
around just because we might need it at some point.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5