feat(contributing): add SFTP configuration#8
feat(contributing): add SFTP configuration#8jnijdam wants to merge 4 commits intosaltstack-formulas:masterfrom
Conversation
jnijdam
force-pushed
the
feature/jn/add-sftp
branch
5 times, most recently
from
fc63c80 to
a38031d
Compare
jnijdam
force-pushed
the
feature/jn/add-sftp
branch
6 times, most recently
from
4f610b3 to
d9407e4
Compare
There was a problem hiding this comment.
I made several comments to the code.
As noted, for now this code makes a breaking change because sftp module is activated which it was not previously.
Another problem is, when activating the sftp state (in kitchen.yml) converge do not pass. I think kitchen/inspec tests should be added to be confident in how this code works. See kitchen.yml suite section to add one to the default one.
jnijdam
force-pushed
the
feature/jn/add-sftp
branch
16 times, most recently
from
2da6cf2 to
55abc2c
Compare
jnijdam
force-pushed
the
feature/jn/add-sftp
branch
13 times, most recently
from
8e79756 to
1011e78
Compare
daks
left a comment
daks
left a comment
There was a problem hiding this comment.
You added a Kitchen test suite, some Inspec tests but those are not activated in Gitlab CI so no way to see it tests passes.
proftpd/init.sls
Outdated
| - group: root | ||
| - mode: 644 | ||
| - template: jinja | ||
| - context: |
There was a problem hiding this comment.
I'm not sure you need to define both context and defaults, stick with context it should be sufficient
There was a problem hiding this comment.
I confirme, context is enough thanks
There was a problem hiding this comment.
ok, in reality previously defaults was used. I don't think it changes anything but by security maybe it's better to let the previous one in place.
@myii any advice maybe?
| its('content') { should include 'SFTPHostKeyrsa' } | ||
| its('content') { should include 'SFTPAuthMethods' } | ||
| its('content') { should include 'SFTPAuthorizedUserKeys' } | ||
| its('content') { should include 'Port 4000' } |
There was a problem hiding this comment.
the number of spaces between Port and 4000 is weird, any reason to that?
There was a problem hiding this comment.
this is what we expect to find in the final sftp.conf file.
I tried to reduce these spaces and the check fails
jnijdam
force-pushed
the
feature/jn/add-sftp
branch
9 times, most recently
from
51c0fe5 to
cd6e145
Compare
daks
left a comment
daks
left a comment
There was a problem hiding this comment.
looks quite now and could be merged soon. One thing missing is to update pillar.example, can you please add it?
proftpd/init.sls
Outdated
| - group: root | ||
| - mode: 644 | ||
| - template: jinja | ||
| - context: |
There was a problem hiding this comment.
ok, in reality previously defaults was used. I don't think it changes anything but by security maybe it's better to let the previous one in place.
@myii any advice maybe?
|
@jnijdam I'm working on improving your PR which is not functional right now. I'll add commits to it once I get a working setup |
2 participants
PR progress checklist (to be filled in by reviewers)
What type of PR is this?
Primary type
[feat]A new featureSecondary type
[docs]Documentation changes[test]Adding missing or correcting existing testsDoes this PR introduce a
BREAKING CHANGE?No.
Related issues and/or pull requests
Describe the changes you're proposing
Pillar / config required to test the proposed changes
Debug log showing how the proposed changes work
Documentation checklist
README(e.g.Available states).Testing checklist
state_top).Additional context