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

Pillar fixes #217

Merged
merged 9 commits into from
Apr 11, 2019
Merged

Pillar fixes #217

merged 9 commits into from
Apr 11, 2019

Conversation

garrettw
Copy link
Contributor

@garrettw garrettw commented Apr 10, 2019 •
edited
Loading

Just found a few issues in pillar.example as I was going through it adapting it to my own needs.

noelmcloughlin
noelmcloughlin approved these changes Apr 10, 2019
View reviewed changes
Copy link
Member

@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 @garrettw

@garrettw
Copy link
Contributor Author

For future reference: 000c96a fixes code added in #211 (specifically 8921404)

@garrettw garrettw changed the title Pillar fixes Server config fixes Apr 10, 2019
myii
myii reviewed Apr 10, 2019
View reviewed changes
pillar.example Show resolved Hide resolved
myii
myii reviewed Apr 10, 2019
View reviewed changes
nginx/ng/map.jinja Outdated Show resolved Hide resolved
myii
myii requested changes Apr 10, 2019
View reviewed changes
Copy link
Member

@myii myii left a comment

Choose a reason for hiding this comment

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

Needs to be checked by someone before merging this PR.

pillar.example Outdated Show resolved Hide resolved
@daks
Copy link
Member

daks commented Apr 11, 2019

I wonder how the error in pillar.example https://github.com/saltstack-formulas/nginx-formula/pull/217/files#diff-aef06cd118a72975a36aa9aeb06e8af6R121 make the tests successful.
Tests are implemented with a different framework that what I'm (we are?) used to, and I don't know how it works.

@myii
Copy link
Member

myii commented Apr 11, 2019

@daks (From information collected in saltstack-formulas/template-formula#21). This formula is using the unmaintained formula-test-harness, implemented in #186. We had a a discussion about it on Slack, outlining why we're no longer considering this for future use.

@garrettw garrettw changed the title Server config fixes Pillar fixes Apr 11, 2019
@daks
Copy link
Member

daks commented Apr 11, 2019

@myii do you mean if I find the courage and time to write those, I can submit a PR? How is it better to wait for a future refactoring of the formula (switching to versioning, to get rid of nginx.ng)?

myii
myii reviewed Apr 11, 2019
View reviewed changes
pillar.example Outdated Show resolved Hide resolved
@myii
Copy link
Member

myii commented Apr 11, 2019

@myii do you mean if I find the courage and time to write those, I can submit a PR? How is it better to wait for a future refactoring of the formula (switching to versioning, to get rid of nginx.ng)?

@daks The current testing method doesn't work at all so we don't have to wait for a future refactor to fix this. If you want to replace this with inspec, that would be a very welcome effort! My angle is that nginx.ng is the priority, since that is what will become v1.0.0. @javierbertoli Any comments about this?

@myii myii merged commit 62e8ac0 into saltstack-formulas:master Apr 11, 2019
@myii
Copy link
Member

myii commented Apr 11, 2019

@garrettw Thanks for your patience. This PR has been (squashed and) merged!

Note: As we discussed on Slack, we will get back to log_format in a subsequent (draft/WIP) PR.

@garrettw garrettw deleted the pillar_fixes branch April 11, 2019 16:52
@garrettw garrettw mentioned this pull request Apr 11, 2019
Merged
@daks
Copy link
Member

daks commented Apr 12, 2019

@myii OK. I'm also thinking of only supporting nginx.ng which will become the default once we add versioning.

blarghmatey pushed a commit to mitodl/nginx-formula that referenced this pull request Dec 17, 2019
@garrettw @blarghmatey
Pillar fixes (saltstack-formulas#217)
03e0947 
Squashed from:

* pillar.example: an include was missing a colon
* pillar.example: 2 directives not indented properly
* pillar.example: added the stock log_format
* pillar.example: make log path match map.jinja
* pillar.example: worker_connections value from latest mainline nginx
* pillar.example: fixed other uses of include
* pillar.example: corrected formatting
* pillar.example: corrected log_format
* pillar.example: reset log_format to try another day
markbreedlove pushed a commit to mitodl/nginx-formula that referenced this pull request Dec 17, 2019
@garrettw
Pillar fixes (saltstack-formulas#217)
617eb7a 
Squashed from:

* pillar.example: an include was missing a colon
* pillar.example: 2 directives not indented properly
* pillar.example: added the stock log_format
* pillar.example: make log path match map.jinja
* pillar.example: worker_connections value from latest mainline nginx
* pillar.example: fixed other uses of include
* pillar.example: corrected formatting
* pillar.example: corrected log_format
* pillar.example: reset log_format to try another day
@myii myii mentioned this pull request Dec 20, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@myii myii myii requested changes

@noelmcloughlin noelmcloughlin noelmcloughlin approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@garrettw @daks @myii @noelmcloughlin