Skip to content

fix(snippet-file-name): removed appending of ".conf" for snippets files #240

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

Merged

Conversation

sticky-note
Copy link
Member

@sticky-note sticky-note commented Jun 17, 2019

Since introduction of TOFS in this formula, we can retrieve file that are not described by pillar. TOFS will generate source like the following:

nginx_snippet_letsencrypt:
              source:
                  - salt://nginx/files/snippets/letsencrypt
                  - salt://nginx/files/snippets/server.conf
                  - salt://nginx/files/A/B/C/letsencrypt
                  - salt://nginx/files/A/B/C/server.conf
                  - salt://nginx/files/FreeBSD/letsencrypt
                  - salt://nginx/files/FreeBSD/server.conf
                  - salt://nginx/files/default/letsencrypt
                  - salt://nginx/files/default/server.conf

However, we have not same behavior between snippets file and server_configs.
Actually formula, append .conf as an extension for all snippets.
If we include snippets/*.conf in the main nginx.conf for example, it is not always the case that we desire to include all snippets.
This PR propose to remove the appending of '.conf' for snippets files and permit to choose file name and extension for snippets with snippet name.

Copy link
Contributor

@noelmcloughlin noelmcloughlin left a comment

Choose a reason for hiding this comment

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

Thanks @sticky-note
I cannot see any technical issues with the code.
Could you update the PR description to elaborate on what problem is being fixed?

@daks
Copy link
Member

daks commented Jun 17, 2019

Hello, I think this PR introduces a 'BREAKING CHANGE' because people must adapt their pillar. This must be explicitely stated in the commit message, can you update it?

@sticky-note sticky-note force-pushed the fix/snippets-file-name branch from 6150a3c to 5553c21 Compare June 17, 2019 21:21
@sticky-note
Copy link
Member Author

@daks @noelmcloughlin updated, is that ok?

@daks
Copy link
Member

daks commented Jun 18, 2019

Not sure the commit 'BREAKING CHANGE' message is formatted correctly https://github.com/saltstack-formulas/nginx-formula/blob/master/docs/CONTRIBUTING.rst#use-breaking-change-to-trigger-a-major-version-change

otherwise good for me, nice catch @sticky-note :)

@sticky-note sticky-note force-pushed the fix/snippets-file-name branch from 5553c21 to aa87721 Compare June 19, 2019 02:13
@sticky-note
Copy link
Member Author

@daks If I read correctly, it is ok now. Sorry for multiple editing.

Copy link
Contributor

@noelmcloughlin noelmcloughlin left a comment

Choose a reason for hiding this comment

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

Thanks @sticky-note for the hard work and responsiveness & @daks for reviewing

@noelmcloughlin noelmcloughlin merged commit f9000dd into saltstack-formulas:master Jun 19, 2019
@myii
Copy link
Contributor

myii commented Jun 19, 2019

Thanks to everyone for this PR. For reference, this PR links back to #238 (comment).

@saltstack-formulas-travis

🎉 This PR is included in version 2.0.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

(...) for snippets filename.
BREAKING CHANGE: Users have to modify their pillar
according to this commit. Users MUST append '.conf' for their
existing managed snippets.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants