Skip to content

Conversation

@sticky-note
Copy link
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

Copy link
Member

@daks daks left a comment

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 :)

@myii
Copy link
Contributor

myii commented Jun 25, 2019

@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
Member Author

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

Otherwise LGTM, good job :)

Added docs/TOFS-pattern.rst

@sticky-note
Copy link
Member Author

sticky-note commented Jul 1, 2019

@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
Copy link
Contributor

myii commented Jul 3, 2019

@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
Copy link
Contributor

myii commented Jul 4, 2019

@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
Copy link
Contributor

myii commented Jul 4, 2019

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
Copy link
Member Author

sticky-note commented Jul 4, 2019

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

@myii myii merged commit 193ff7e into saltstack-formulas:master Jul 6, 2019
@myii
Copy link
Contributor

myii commented Jul 6, 2019

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