Skip to content

feat(TOFS): ssh sshd configs known_host and banner#164

Merged
myii merged 2 commits into
saltstack-formulas:masterfrom
sticky-note:feat/tofs
Jul 6, 2019
Merged

feat(TOFS): ssh sshd configs known_host and banner#164
myii merged 2 commits into
saltstack-formulas:masterfrom
sticky-note:feat/tofs

Conversation

@sticky-note

Copy link
Copy Markdown
Member

Here is a contribution to openssh-formula.
Don't know how to handle source filenames here, so I decided to keep backward compatibility.
What do you think about it ?
Ping @myii

@daks daks left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to copy from template-formula the TOFS doc.

Otherwise LGTM, good job :)

Comment thread pillar.example Outdated
@myii

myii commented Jun 25, 2019

Copy link
Copy Markdown
Contributor

@sticky-note These TOFS PRs need to be checked thoroughly, in a similar way that has been done for the other two TOFS PRs we've worked on:

  1. feat(tofs): First implementation nginx-formula#238 (comment)
  2. feat(tofs): implementation for all file.managed php-formula#178 (comment)

Could you please provide this output for this PR?

@sticky-note

Copy link
Copy Markdown
Member Author

You need to copy from template-formula the TOFS doc.

Otherwise LGTM, good job :)

Added docs/TOFS-pattern.rst

@sticky-note

sticky-note commented Jul 1, 2019

Copy link
Copy Markdown
Member Author

@myii, Here is my output for this PR:
salt 'tgt' state.show_sls openssh.banner :

(...)
  sshd_banner:
  (...)
        file:
            |_
              ----------
              name:
                  /etc/ssh/banner
            |_
              ----------
              source:
                  - salt://openssh/files/tgt/banner
                  - salt://openssh/files/FreeBSD/banner
                  - salt://openssh/files/default/banner
(...)

salt 'tgt' state.show_sls openssh.config :

(...)
    ssh_config:
    (...)
        file:
            |_
              ----------
              name:
                  /etc/ssh/ssh_config
            |_
              ----------
              source:
                  - salt://openssh/files/tgt/ssh_config
                  - salt://openssh/files/FreeBSD/ssh_config
                  - salt://openssh/files/default/ssh_config
    (...)
    sshd_config:
    (...)
        file:
            |_
              ----------
              name:
                  /etc/ssh/sshd_config
            |_
              ----------
              source:
                  - salt://openssh/files/tgt/sshd_config
                  - salt://openssh/files/FreeBSD/sshd_config
                  - salt://openssh/files/default/sshd_config
    (...)

salt 'tgt' state.show_sls openssh.known_hosts :

(...)
    manage ssh_known_hosts file:
    (...)
        file:
            |_
              ----------
              name:
                  /etc/ssh/ssh_known_hosts
            |_
              ----------
              source:
                  - salt://openssh/files/tgt/ssh_known_hosts
                  - salt://openssh/files/FreeBSD/ssh_known_hosts
                  - salt://openssh/files/default/ssh_known_hosts
    (...)

@myii

myii commented Jul 3, 2019

Copy link
Copy Markdown
Contributor

@sticky-note Apologies for the delay. Thanks for this PR, I've done most of the checks and it's generally fine. I've got some comments but I will need to come back to this when I've got some more time available.

* Use consistent Jinja whitespace control `{%- ... -}`
* Improve debug output (comments & whitespace control)
* Use exact state names with TOFS `files_switch`
* Add `ssh_known_hosts_src` to `defaults` (for consistency)
* Restrict `pillar.example` changes to TOFS only
* Use `fire_banner` in `pillar.example` to indicate available template
@myii

myii commented Jul 4, 2019

Copy link
Copy Markdown
Contributor

@sticky-note So now it's time for me to turn the tables on you -- it's your turn to review my changes! See the commit that I've added here (f6dbca3). Here's my explanation about what I'm proposing:

  • Use consistent Jinja whitespace control {%- ... -}
  • Improve debug output (comments & whitespace control)
  • Use exact state names with TOFS files_switch
  • Add ssh_known_hosts_src to defaults (for consistency)
  • Restrict pillar.example changes to TOFS only
  • Use fire_banner in pillar.example to indicate available template

Feel free to agree or disagree. I can explain further if necessary. I look forward to your advice.

Note, if all is OK, nothing else needs to be done from your side. The merge can proceed as usual.

@myii

myii commented Jul 4, 2019

Copy link
Copy Markdown
Contributor

For completeness, the diff of changes before and after this PR (after the last commit added).


openssh.banner (non-TOFS)

  • No change.

openssh.banner (TOFS)

-      - salt://openssh/files/banner
+      - salt://openssh/files/ABC/banner
+      - salt://openssh/files/Debian/banner
+      - salt://openssh/files/default/banner

openssh.config (non-TOFS)

  • No change.

openssh.config (TOFS)

-      - salt://openssh/files/sshd_config
+      - salt://openssh/files/ABC/sshd_config
+      - salt://openssh/files/Debian/sshd_config
+      - salt://openssh/files/default/sshd_config

...

-      - salt://openssh/files/ssh_config
+      - salt://openssh/files/ABC/ssh_config
+      - salt://openssh/files/Debian/ssh_config
+      - salt://openssh/files/default/ssh_config

openssh.known_hosts

-      - salt://openssh/files/ssh_known_hosts
+      - salt://openssh/files/ABC/ssh_known_hosts
+      - salt://openssh/files/Debian/ssh_known_hosts
+      - salt://openssh/files/default/ssh_known_hosts

@sticky-note

sticky-note commented Jul 4, 2019

Copy link
Copy Markdown
Member Author

It looks more consistent
I'm okay with these changes, thanks @myii
I'm wondering why you want to remove tofs indications that are not in tofs block
It was intended to indicate the user can use tofs on these files

Comment thread pillar.example Outdated
Comment thread pillar.example Outdated
@myii myii merged commit 193ff7e into saltstack-formulas:master Jul 6, 2019
@myii

myii commented Jul 6, 2019

Copy link
Copy Markdown
Contributor

Merged, thanks for the excellent work and patience, @sticky-note!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants