Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade formula to standards #2

Merged
merged 4 commits into from
Mar 8, 2019
Merged

Upgrade formula to standards #2

merged 4 commits into from
Mar 8, 2019

Conversation

aboe76
Copy link
Member

@aboe76 aboe76 commented Mar 7, 2019

Added tofs pattern from template-formula
Improved map.jinja and defaults
Improved README.rst fix small errors
Improved tofs_pattern.rst
updated sls files with tplroot dir
added the ability to add extra_packages.

@aboe76 aboe76 requested a review from myii March 7, 2019 20:43
lighttpd/config.sls Outdated Show resolved Hide resolved
- source: {{ files_switch(
salt['config.get'](
tplroot ~ ':tofs:files:lighttpd_config',
['/etc/lighttpd/lighttpd.conf', '/etc/lighttpd/lighttpd.conf.jinja']
Copy link
Member

Choose a reason for hiding this comment

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

This is the default template. Since you're only using /etc/lighttpd/lighttpd.conf in the repo, /etc/lighttpd/lighttpd.conf.jinja can be removed.

pillar.example Show resolved Hide resolved
@myii
Copy link
Member

myii commented Mar 7, 2019

@aboe76 That's the eyeball review done. I'll see if I can get a chance to test it at my end as well, a bit later on.

# This is unnecessary in most cases; there are sensible defaults.
# path_prefix: template_alt
# dirs:
# files: files_alt
Copy link
Member

Choose a reason for hiding this comment

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

Something has been bothering me about this files and the files below. I'd like to change the name of one of these across all of the new TOFS before they become too widely used. I think the bottom one should be changed to templates. Is it too late to do this across the formulas where we've started using this?

# dirs:
# files: files_alt
# default: default_alt
# files:
Copy link
Member

Choose a reason for hiding this comment

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

This is the files that I believe should be changed. Otherwise, there could be confusion between the two.

Copy link
Member Author

Choose a reason for hiding this comment

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

there is a difference: the first files is just the name of the folder where the files lives.
a bether name could be folder? or dir,

I guess it the same with the other formulas, we use files in which we house files and templates, other configuration tools use separate places, one files one templates

myii added a commit that referenced this pull request Mar 8, 2019
@myii
Copy link
Member

myii commented Mar 8, 2019

@aboe76 Here's the diff of lighttpd using pillar.example before and after this PR up to this point.

--- before
+++ after
@@ -3,6 +3,43 @@
   "conf_enabled": "/etc/lighttpd/conf-enabled",
   "conf_use_symlink": true,
   "config": "lighttpd.conf",
+  "extra_packages": [],
+  "lookup": {
+    "conf_available": "/etc/lighttpd/conf-available",
+    "conf_enabled": "/etc/lighttpd/conf-enabled",
+    "conf_use_symlink": true,
+    "config": "lighttpd.conf",
+    "package": "lighttpd",
+    "service": "lighttpd"
+  },
+  "modules": {
+    "disabled": [
+      "accesslog",
+      "auth",
+      "cgi",
+      "debian-doc",
+      "dir-listing",
+      "evasive",
+      "evhost",
+      "expire",
+      "extforward",
+      "fastcgi",
+      "fastcgi-php",
+      "flv-streaming",
+      "no-www",
+      "proxy",
+      "rrdtool",
+      "simple-vhost",
+      "ssi",
+      "ssl",
+      "status",
+      "userdir",
+      "usertrack"
+    ],
+    "enabled": [
+      "javascript-alias"
+    ]
+  },
   "package": "lighttpd",
   "service": "lighttpd"
 }

Please confirm that's what you expected. It looks like it is.

Other than that, I need your answer about the tofs dilemma, where files is being used twice. What do you think?

@aboe76
Copy link
Member Author

aboe76 commented Mar 8, 2019

@myii the diff looks good.

@myii
Copy link
Member

myii commented Mar 8, 2019

@aboe76 What's the right solution about files being used twice under tofs?

  • tofs:dirs:files
  • tofs:files

Leave this as-is? That can easily lead to confusion. Which one should change?

@aboe76
Copy link
Member Author

aboe76 commented Mar 8, 2019

@myii it's already confusing for me too, what's the point of the first files

tofs:
  dirs:
    files: bla

@myii
Copy link
Member

myii commented Mar 8, 2019

@aboe76 Are you available on Matrix, it will be easier to resolve this there.

@myii myii merged commit ae752ff into saltstack-formulas:master Mar 8, 2019
@aboe76 aboe76 deleted the update_tofs branch March 8, 2019 21:46
@myii
Copy link
Member

myii commented Mar 8, 2019

@aboe76 Thanks for this. We'll fix TOFS in a subsequent PR, as we've just discussed on Matrix.

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.

2 participants