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

Some configuration improvements #15

Closed
wants to merge 1 commit into from

Conversation

javierbertoli
Copy link
Member

Some suggestions as discussed here.

Copy link
Contributor

@johnkeates johnkeates left a comment

Choose a reason for hiding this comment

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

I’m not sure if this helps much for this template. Basically, anything more advanced than this requires proper engineering anyway.

@javierbertoli
Copy link
Member Author

Yeah, I agree. I just submitted it with the idea of giving, in the template, an example of behaviour to follow when dealing with configuration.

I've seen many formulas modiying config by default and sometimes, specially when they're not up to date with the OS changes, changing configs in ways that can be dangerous (ie, re-enabling SSLv2 or opening ports by default, etc.)

Probably it's better to add these ideas in the docs, right? Or better, also in the docs, as many people that develop formulas, once they get the gist, don't go back much to those docs, and just clone from the template?

Another recommendation regarding config would be that, whenever the package allows it, keep the main config as is, and just override the desired parameters in a local config file.

But as you say, not sure if this is the place to put those things as examples, or the way to do it is just in the docs?

If you consider these ideas ar worthy, I'll be glad to try to add them where you suggest. Otherwise, I'll be OK if this PR is just closed and disregarded.

Copy link
Contributor

@vutny vutny left a comment

Choose a reason for hiding this comment

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

I like the README update reflecting the actual repo structure. Well done!


# If don't you have other config files to manage, remove these lines
# The parameter `manage_another_config must default to False` in
# defaults.yaml
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we could go ahead and simply add those parameters (manage_another_config, another_config) to the defaults.yaml file? 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if that is a good idea, this basically assumes that someone will use the template to create a formula with optional configuration.

@johnkeates
Copy link
Contributor

johnkeates commented Mar 20, 2018

It is true that unmaintained formulas might modify configuration files in a way that is undesirable, but I don't think it is a good idea to try and blanket-disable all the configuration states in all formulas. Surely most people doing advanced stuff won't even use the template, but those who do are already having trouble the the concept of having map.jinja and defaults.yaml plus the lookup tree in the pillar on top of everything else.

It would probably be a good thing if this were indeed put in the docs, maybe some sort of warning to make sure you know what the forumla is going to do before just blindly including everything.

In many cases a simple fun of init.sls with basic settings will work just fine, and in those cases often the desired result aligns with what the template does:

  • install stuff
  • configure stuff
  • enable stuff
  • run stuff

If at some point a user would want something else, they would already be at the point where they pick and choose states anyway (i.e. run template.install and maybe template.service but not template.config).

To actually fix the issue with degraded formulas and stability, we would have to fix the whole repository deal as a complete thing, for example, getting SPM and a package management system up, maybe in NPM or Composer style where the packages are still mostly referenced from github, but the stuff you get from the package manager is versioned, checked, and perhaps gives warnings if you add a package that is unmaintained, has no tests like kitchen salt+serverspec, or perhaps has produced errors on a test run in general.

If you were to want to go ahead and have a more 'managed' template, I'd suggest adding one or more repositories, containing various examples and docs for formulas, and pointing to this one to refer to a basic traditional formula, hosted and named in the style of saltstack-formulas.
We could call it 'templates' in the saltstack-formulas namespace so it is super clear what it is for, and then make sure the top level directory contains no formulas or states at all to prevent new users from accidentally cloning and trying to run it.

@vutny
Copy link
Contributor

vutny commented Mar 21, 2018

Those things are absolutely valid. The scope of getting it done is just to broad for a single PR.

For now I think we could leave config.sls as is, but at very least it would be helpful for anyone to put description of each already existing state to README. This is a good practice to follow and guide others. Also upstream docs encourage us to do so.

@javierbertoli
Copy link
Member Author

Closing, as the comments suggest keeping config.sls as is and eventually improve templates/documentation in other ways.

@johnkeates
Copy link
Contributor

We should open a 2nd ticket or repo with the more advanced template techniques as it is definitely valuable. While putting formula stuff in Salt's main docs is a good idea, the issue there is probably that formulas are rather free-form and constraining them from the get-go is maybe not the best way to do things. On the other hand, putting out best practises is probably fine. Do we split this off into a separate repo or do we raise an issue with Salt first?

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