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.

Sign up for GitHub

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

Jump to bottom

feat(config): use separate defaults for all states #247

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

feat(config): use separate defaults for all states #247

wants to merge 2 commits into from

Conversation

ErisDS
Copy link
Contributor

@ErisDS ErisDS commented Jul 23, 2019

At the moment this is untested and pure opinion - again happy to do more work if other people agree.

IMO it only makes sense that the defaults are shared, whilst the default templates are in use to reduce duplication.

As soon as you override one of the shared defaults (nginx.conf or server.conf) unexpected things happen:

e.g

  • I specify a custom hard-coded nginx.conf file that I use on all servers. I don't expect it to suddenly be impossible to configure passenger.
  • Similarly, if I do something custom with server.conf, I don't expect that snippers are affected`

This can currently be worked around by specifying your own names for files in tofs.source_files, so the change may be undesirable.

This tripped me up so thought I'd raise to bring it to attention of maintainers.

ErisDS added 2 commits July 23, 2019 14:14
@ErisDS
feat(passenger): inc config, snippets, servers, etc
e07b558 
- currently you're forced to define extra states if you opt to install nginx with passenger,
 if you want the same outcome for passenger and nginx installs with equivalent config
- passenger is an extra module on top of nginx, makes no sense to end up with less configuration by default
@ErisDS
feat(config): use separate defaults for all states
1ceb3f7 
- In the case that you want to use a custom template for one state, it's _unlikely_ that you want to use the same custom template for another
- Although this results in duplicate files, it makes the behaviour of the formula easier to understand, e.g
- I specify a custom hard-coded nginx.conf file that I use on all servers. I don't expect it to suddenly be impossible to configure passenger.
- Similarly, if I do something custom with server.conf, I don't expect that snippers are affected
@ErisDS
Copy link
Contributor Author

ErisDS commented Jul 23, 2019

I take it back, this cannot be worked around. See the context in slack here: https://saltstackcommunity.slack.com/team/ULFR9ES64

TL;DR if you override the template name with source files, it completely overrides the file switch.

Example here: https://github.com/TryGhost/nginx-formula/blob/e957a022763db369dece479479301edaa7223c91/nginx/servers_config.sls#L116

I expected that overriding source_files would change the cascade from looking for files with the dynamic server name, and then server.conf to looking for files the dynamic server name, and then my configured value in source files.

E.g server.conf is replaced with whatever source_files.server_conf_file_managed is configured to.

Instead, it gets rid of the ability to look for the dynamic server name, and only looks for the hardcoded source_files.server_conf_file_managed value.

@ErisDS
Copy link
Contributor Author

ErisDS commented Jul 23, 2019

Bottom line - the reuse of default templates for multiple states is a severe limitation, you cannot override one default without changing the behaviour of another state.

There is no workaround or fix. Meaning you effectively cannot customise the defaults in any way that is intended to tailor them to one state or another - a severe limitation.

myii added a commit to myii/template-formula that referenced this pull request Jul 23, 2019
@myii
fix(tofs): prepend the config-based source_files to the default
b778901 
* saltstack-formulas/nginx-formula#247 (comment)
  - The main issue is that the `nginx-formula` has dynamic values being
    used by as the default `source_files` -- there is no way to provide
    this from the pillar/config in a sensible fashion
  - Prepending to this default (rather than overriding it) resolves this
    problem entirely, without adding excessive entries to the `source`
* Closes saltstack-formulas#151
@myii myii mentioned this pull request Jul 23, 2019
Merged
@myii
Copy link
Member

myii commented Jul 23, 2019

As discussed in Slack, merging saltstack-formulas/template-formula#152 (and propagating to all formulas) will alleviate these problems.

myii added a commit to myii/template-formula that referenced this pull request Jul 23, 2019
@myii
fix(tofs): prepend the config-based source_files to the default
88ecbbe 
* saltstack-formulas/nginx-formula#247 (comment)
  - The main issue is that the `nginx-formula` has dynamic values being
    used as the default `source_files` -- there is no way to provide
    this from the pillar/config in a sensible fashion
  - Prepending to this default (rather than overriding it) resolves this
    problem entirely, without adding excessive entries to the `source`
* Closes saltstack-formulas#151
@ErisDS
Copy link
Contributor Author

ErisDS commented Jul 23, 2019

Yes saltstack-formulas/template-formula#152 will mean there is at least a workaround. However, it would be ideal if no workaround was necessary :)

myii added a commit to myii/template-formula that referenced this pull request Jul 23, 2019
@myii
fix(tofs): prepend the config-based source_files to the default
cb1f1bf 
* saltstack-formulas/nginx-formula#247 (comment)
  - The main issue is that the `nginx-formula` has dynamic values being
    used as the default `source_files` -- there is no way to provide
    this from the pillar/config in a sensible fashion
  - Prepending to this default (rather than overriding it) resolves this
    problem entirely, without adding excessive entries to the `source`
* Closes saltstack-formulas#151
myii added a commit to myii/template-formula that referenced this pull request Jul 25, 2019
@myii
fix(tofs): prepend the config-based source_files to the default
3483e76 
* saltstack-formulas/nginx-formula#247 (comment)
  - The main issue is that the `nginx-formula` has dynamic values being
    used as the default `source_files` -- there is no way to provide
    this from the pillar/config in a sensible fashion
  - Prepending to this default (rather than overriding it) resolves this
    problem entirely, without adding excessive entries to the `source`
* Closes saltstack-formulas#151
saltstack-formulas-travis pushed a commit to saltstack-formulas/template-formula that referenced this pull request Jul 25, 2019
@semantic-release-bot
chore(release): 3.1.1 [skip ci]
10e23bc 
## [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)
@myii myii mentioned this pull request Jul 25, 2019
Merged
@myii
Copy link
Member

myii commented Jul 25, 2019

The merge of #248 has brought in all the TOFS changes as mentioned above.

@aboe76
Copy link
Member

aboe76 commented Sep 1, 2019

@ErisDS can you please take a look and rebase your pr?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ErisDS @myii @aboe76