feat(tofs): First implementation #238

sticky-note · 2019-05-19T21:47:36Z

First implementation of TOFS for:

nginx_config,
passenger_config,
servers_config
snippets

Introduced tplroot on modified files as well
Fixed GH link of nginx.conf file in docs/TOFS_pattern.rst

Edited

myii · 2019-05-19T22:43:11Z

@sticky-note Thanks for this PR.

Initial comments, I haven't looked at this in any detail yet:

Please provide the tofs section in pillar.example, customised to show how to use for these states that have been converted.
To pass the commitlint, you'll need:

-feat(tofs): First imp. for servers_config and snippets
+feat(tofs): first imp. for servers_config and snippets

myii · 2019-05-26T19:22:36Z

Thanks for this @sticky-note, it's definitely taking shape. I've worked through from each of the entry points, comparing before and after. I've used pillar.example with a couple of minor changes:

Disabled nginx:server:config:source_path.
Set nginx:install_from_source when it was needed.

In a few comments below, I've recommended reducing the number of template file names. Ultimately, the user is able to customise these using the nginx:tofs:source_files:... lookup values set in the pillar/config. Perhaps it's best to actually stick to only one offered template (i.e. the existing one) and leave the rest for users to select.

There's obviously some duplicates below, due to formula execution and loops that are used. These will only require fixing in one place.

`nginx.config`

     - name: /etc/nginx/nginx.conf
-    - source: salt://nginx/files/nginx.conf
+    - source:
+      - salt://nginx/files/ABC/nginx.conf
+      - salt://nginx/files/ABC/nginx.conf.jinja
+      - salt://nginx/files/Debian/nginx.conf
+      - salt://nginx/files/Debian/nginx.conf.jinja
+      - salt://nginx/files/default/nginx.conf
+      - salt://nginx/files/default/nginx.conf.jinja

Fine, may want to consider removing nginx.conf.jinja.

`nginx.passenger`

     - name: /etc/nginx/conf.d/mod-http-passenger.conf
-    - source: salt://nginx/files/nginx.conf
+    - source:
+      - salt://nginx/files/ABC/nginx.conf
+      - salt://nginx/files/ABC/nginx.conf.jinja
+      - salt://nginx/files/Debian/nginx.conf
+      - salt://nginx/files/Debian/nginx.conf.jinja
+      - salt://nginx/files/default/nginx.conf
+      - salt://nginx/files/default/nginx.conf.jinja

Fine, may want to consider removing nginx.conf.jinja.

`nginx.servers`

     - name: /etc/nginx/sites-available/mysite
-    - source: salt://nginx/files/server.conf
+    - source:
+      - salt://nginx/files/ABC/mysite
+      - salt://nginx/files/ABC/mysite.conf
+      - salt://nginx/files/ABC/server.conf
+      - salt://nginx/files/ABC/server.conf.jinja
+      - salt://nginx/files/Debian/mysite
+      - salt://nginx/files/Debian/mysite.conf
+      - salt://nginx/files/Debian/server.conf
+      - salt://nginx/files/Debian/server.conf.jinja
+      - salt://nginx/files/default/mysite
+      - salt://nginx/files/default/mysite.conf
+      - salt://nginx/files/default/server.conf
+      - salt://nginx/files/default/server.conf.jinja

I think four template name options are excessive here, leading to unnecessary I/O lookups -- reducing to two should be sufficient.

`nginx.servers_config`

     - name: /etc/nginx/sites-available/mysite
-    - source: salt://nginx/files/server.conf
+    - source:
+      - salt://nginx/files/ABC/mysite
+      - salt://nginx/files/ABC/mysite.conf
+      - salt://nginx/files/ABC/server.conf
+      - salt://nginx/files/ABC/server.conf.jinja
+      - salt://nginx/files/Debian/mysite
+      - salt://nginx/files/Debian/mysite.conf
+      - salt://nginx/files/Debian/server.conf
+      - salt://nginx/files/Debian/server.conf.jinja
+      - salt://nginx/files/default/mysite
+      - salt://nginx/files/default/mysite.conf
+      - salt://nginx/files/default/server.conf
+      - salt://nginx/files/default/server.conf.jinja

Same as above.
~~TODO~~ (Fix nginx.servers_config #239): Note, there's a bug here that needs to be reported or fixed in a separate PR; when attempting to call nginx.servers_config directly (as is available from the README):
- Error: Cannot extend ID 'nginx_service' in 'base:nginx.servers_config'. It is not part of the high state.
- Workaround: Comment out these two lines.

`nginx.service`

     - name: /lib/systemd/system/nginx.service
-    - source: salt://nginx/files/nginx.service
+    - source:
+      - salt://nginx/files/ABC/nginx.service
+      - salt://nginx/files/ABC/nginx.service.jinja
+      - salt://nginx/files/Debian/nginx.service
+      - salt://nginx/files/Debian/nginx.service.jinja
+      - salt://nginx/files/default/nginx.service
+      - salt://nginx/files/default/nginx.service.jinja

Fine, may want to consider removing nginx.service.jinja.

`nginx.snippets` -- `blacklist`

-    - name: /etc/nginx/snippets/blacklist.conf
-    - source: salt://nginx/files/server.conf
+    - name: /etc/nginx/snippets/blacklist
+    - source:
+      - salt://nginx/files/ABC/blacklist
+      - salt://nginx/files/ABC/blacklist.jinja
+      - salt://nginx/files/ABC/server.conf
+      - salt://nginx/files/ABC/server.conf.jinja
+      - salt://nginx/files/Debian/blacklist
+      - salt://nginx/files/Debian/blacklist.jinja
+      - salt://nginx/files/Debian/server.conf
+      - salt://nginx/files/Debian/server.conf.jinja
+      - salt://nginx/files/default/blacklist
+      - salt://nginx/files/default/blacklist.jinja
+      - salt://nginx/files/default/server.conf
+      - salt://nginx/files/default/server.conf.jinja

Keep the name the same as before, i.e. /etc/nginx/snippets/blacklist.conf.
Same as above in respect to reducing to two template names at most.

`nginx.snippets` -- `letsencrypt`

-    - name: /etc/nginx/snippets/letsencrypt.conf
-    - source: salt://nginx/files/server.conf
+    - name: /etc/nginx/snippets/letsencrypt
+    - source:
+      - salt://nginx/files/ABC/letsencrypt
+      - salt://nginx/files/ABC/letsencrypt.jinja
+      - salt://nginx/files/ABC/server.conf
+      - salt://nginx/files/ABC/server.conf.jinja
+      - salt://nginx/files/Debian/letsencrypt
+      - salt://nginx/files/Debian/letsencrypt.jinja
+      - salt://nginx/files/Debian/server.conf
+      - salt://nginx/files/Debian/server.conf.jinja
+      - salt://nginx/files/default/letsencrypt
+      - salt://nginx/files/default/letsencrypt.jinja
+      - salt://nginx/files/default/server.conf
+      - salt://nginx/files/default/server.conf.jinja

Same as above.

`nginx.snippets` -- `cloudflare_proxy`

-    - name: /etc/nginx/snippets/cloudflare_proxy.conf
-    - source: salt://nginx/files/server.conf
+    - name: /etc/nginx/snippets/cloudflare_proxy
+    - source:
+      - salt://nginx/files/ABC/cloudflare_proxy
+      - salt://nginx/files/ABC/cloudflare_proxy.jinja
+      - salt://nginx/files/ABC/server.conf
+      - salt://nginx/files/ABC/server.conf.jinja
+      - salt://nginx/files/Debian/cloudflare_proxy
+      - salt://nginx/files/Debian/cloudflare_proxy.jinja
+      - salt://nginx/files/Debian/server.conf
+      - salt://nginx/files/Debian/server.conf.jinja
+      - salt://nginx/files/default/cloudflare_proxy
+      - salt://nginx/files/default/cloudflare_proxy.jinja
+      - salt://nginx/files/default/server.conf
+      - salt://nginx/files/default/server.conf.jinja

Same as above.

`nginx.snippets` -- `upstream_netdata_tcp`

-    - name: /etc/nginx/snippets/upstream_netdata_tcp.conf
-    - source: salt://nginx/files/server.conf
+    - name: /etc/nginx/snippets/upstream_netdata_tcp
+    - source:
+      - salt://nginx/files/ABC/upstream_netdata_tcp
+      - salt://nginx/files/ABC/upstream_netdata_tcp.jinja
+      - salt://nginx/files/ABC/server.conf
+      - salt://nginx/files/ABC/server.conf.jinja
+      - salt://nginx/files/Debian/upstream_netdata_tcp
+      - salt://nginx/files/Debian/upstream_netdata_tcp.jinja
+      - salt://nginx/files/Debian/server.conf
+      - salt://nginx/files/Debian/server.conf.jinja
+      - salt://nginx/files/default/upstream_netdata_tcp
+      - salt://nginx/files/default/upstream_netdata_tcp.jinja
+      - salt://nginx/files/default/server.conf
+      - salt://nginx/files/default/server.conf.jinja

Same as above.

myii

As outlined in #238 (comment).

myii · 2019-05-26T19:25:11Z

CC: @aboe76 @noelmcloughlin @daks.

Since this is a fairly significant PR, I would appreciate it if any of you could take a look.

myii · 2019-05-26T19:26:17Z

@sticky-note Please include this fix as well: saltstack-formulas/template-formula#124.

myii · 2019-05-26T19:36:58Z

! Forumla don't append '.conf' on snippets files any more in order to have the same behavior of nginx:servers:managed. !

My suggestion is to keep this PR as a pure refactor and then apply this in a later PR.

sticky-note · 2019-05-27T06:55:48Z

Thanks @myii for this review, I updated PR according to your suggestions .
A question remain pending : Is this a good idea to make lookup depending on server name on the the two reviews below ?

myii

This is definitely taking shape now and just a few changes from my side -- great work.

docs/TOFS_pattern.rst

nginx/servers.sls

nginx/service.sls

pillar.example

myii · 2019-05-30T23:52:00Z

nginx/servers_config.sls

-{% else %}
-{% set source_path = 'salt://nginx/files/server.conf' %}
-{% endif %}
+{% if settings.enabled == True %}


I haven't looked into this but is there a reason you removed the settings.config != None?

I removed this to be able to not specify a config object so that flat files can be retrieved by TOFS without this param set empty

myii · 2019-05-30T23:52:11Z

nginx/servers_config.sls

@@ -138,7 +144,7 @@ nginx_server_available_dir:
 {% else %}
 {{ manage_status(server, settings.enabled, False) }}
 {% endif %}
-{% if settings.config != None and settings.enabled == True %}
+{% if settings.enabled == True %}


I haven't looked into this but is there a reason you removed the settings.config != None?

I removed this to be able to not specify a config object so that flat files can be retrieved by TOFS without this param set empty

myii · 2019-05-30T23:55:23Z

pillar.example

@@ -84,6 +85,8 @@ nginx:
      - upstream netdata:
        - server: 127.0.0.1:19999
        - keepalive: 64
+    file_retrieved_by_tofs: {} # File retrieved by TOFS,


How would this be used exactly?

Don't know how to explain that, it will search files :

nginx/files/ABC/file_retrieved_by_tofs nginx/files/ABC/server.conf nginx/files/default/file_retrieved_by_tofs nginx/files/default/server.conf

without modifying TOFS parameters

Isn't this already provided by the tofs section of the pillar? I.e.:

tofs: ... source_files: ... nginx_snippet_file_managed: - file_retrieved_by_tofs - server.conf

This is the default behavior without specifying anything in pillar

pillar.example

myii

Still a few more issues! Don't worry, this happens with large, important PRs.

pillar.example

myii · 2019-06-03T08:25:43Z

pillar.example

+    # source_files:
+    #   template-config-file-file-managed:
+    #     - 'example_alt.tmpl'
+    #     - 'example_alt.tmpl.jinja'


These four lines should be removed since the section is shown below.

myii · 2019-06-03T08:26:39Z

pillar.example

+      - role
+      - osfinger
+      - os
+      - os_family


This files_switch section is better commented out since we don't really want the extra I/O unless necessary.

myii · 2019-06-03T08:31:34Z

pillar.example

@@ -84,6 +85,8 @@ nginx:
      - upstream netdata:
        - server: 127.0.0.1:19999
        - keepalive: 64
+    file_retrieved_by_tofs: {} # File retrieved by TOFS,


Isn't this already provided by the tofs section of the pillar? I.e.:

tofs: ... source_files: ... nginx_snippet_file_managed: - file_retrieved_by_tofs - server.conf

myii · 2019-06-03T08:34:42Z

@aboe76 @noelmcloughlin @daks I'd really appreciate a second review on this PR. It's pretty much complete now so can be tested to ensure it is satisfactory.

daks · 2019-06-03T09:59:58Z

@myii I'll try to find some time to review this PR, but I've no real knowledge of TOFS :-/

First implementation of TOFS for: nginx_config, passenger_config, servers_config and snippets Introduced tplroot on modified files as well Fixed GH link of nginx.conf file in docs/TOFS_pattern.rst Fixed test for snippets name pillar

myii · 2019-06-03T22:24:28Z

Brilliant work @sticky-note -- this has been merged.
No problem @daks, I've already done a number of test runs, thanks for the offer.

saltstack-formulas-travis · 2019-06-03T22:41:18Z

🎉 This PR is included in version 1.1.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

* saltstack-formulas/nginx-formula#238 (comment)

## [3.1.1](v3.1.0...v3.1.1) (2019-07-25) ### Bug Fixes * **tofs:** prepend the config-based `source_files` to the default ([3483e76](3483e76)), closes [/github.com/saltstack-formulas/nginx-formula/pull/247#issuecomment-514262549](https://github.com//github.com/saltstack-formulas/nginx-formula/pull/247/issues/issuecomment-514262549) [#151](#151) ### Documentation * **tofs:** ensure merged will all recent changes ([6a614d9](6a614d9)) * **tofs:** update from `nginx-formula` ([23a221e](23a221e)), closes [/github.com/saltstack-formulas/nginx-formula/pull/238#discussion_r289124365](https://github.com//github.com/saltstack-formulas/nginx-formula/pull/238/issues/discussion_r289124365)

sticky-note changed the title ~~feat(tofs): First imp. for servers_config and snippets~~ feat(tofs): First implementation May 24, 2019

myii suggested changes May 26, 2019

View reviewed changes

myii mentioned this pull request May 26, 2019

fix(libtofs): use select to deal with empty strings in path saltstack-formulas/template-formula#124

Merged

myii mentioned this pull request May 27, 2019

Fix nginx.servers_config #239

Open

myii suggested changes May 30, 2019

View reviewed changes

myii suggested changes Jun 3, 2019

View reviewed changes

feat(tofs): first implemetation + tplroot

d5262ea

First implementation of TOFS for: nginx_config, passenger_config, servers_config and snippets Introduced tplroot on modified files as well Fixed GH link of nginx.conf file in docs/TOFS_pattern.rst Fixed test for snippets name pillar

myii approved these changes Jun 3, 2019

View reviewed changes

myii merged commit f036e31 into saltstack-formulas:master Jun 3, 2019

saltstack-formulas-travis added the released label Jun 3, 2019

myii added a commit to saltstack-formulas/template-formula that referenced this pull request Jul 25, 2019

docs(tofs): update from nginx-formula

23a221e

* saltstack-formulas/nginx-formula#238 (comment)

myii mentioned this pull request Mar 17, 2021

[BUG] How to set internal property on pillar? #279

Closed

feat(tofs): First implementation #238

feat(tofs): First implementation #238

Uh oh!

Conversation

sticky-note commented May 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

myii commented May 19, 2019

Uh oh!

myii commented May 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

nginx.config

nginx.passenger

nginx.servers

nginx.servers_config

nginx.service

nginx.snippets -- blacklist

nginx.snippets -- letsencrypt

nginx.snippets -- cloudflare_proxy

nginx.snippets -- upstream_netdata_tcp

Uh oh!

myii left a comment

Choose a reason for hiding this comment

Uh oh!

myii commented May 26, 2019

Uh oh!

myii commented May 26, 2019

Uh oh!

myii commented May 26, 2019

Uh oh!

sticky-note commented May 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

myii left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sticky-note Jun 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

myii left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

myii commented Jun 3, 2019

Uh oh!

daks commented Jun 3, 2019

Uh oh!

myii commented Jun 3, 2019

Uh oh!

saltstack-formulas-travis commented Jun 3, 2019

Uh oh!

Uh oh!

sticky-note commented May 19, 2019 •

edited

Loading

myii commented May 26, 2019 •

edited

Loading

`nginx.config`

`nginx.passenger`

`nginx.servers`

`nginx.servers_config`

`nginx.service`

`nginx.snippets` -- `blacklist`

`nginx.snippets` -- `letsencrypt`

`nginx.snippets` -- `cloudflare_proxy`

`nginx.snippets` -- `upstream_netdata_tcp`

sticky-note commented May 27, 2019 •

edited

Loading

sticky-note Jun 2, 2019 •

edited

Loading