Skip to content

fix: prevent running of states deprecated in v1.0.0#236

Merged
myii merged 1 commit into
saltstack-formulas:masterfrom
myii:bug/prevent-deprecated-states
May 13, 2019
Merged

fix: prevent running of states deprecated in v1.0.0#236
myii merged 1 commit into
saltstack-formulas:masterfrom
myii:bug/prevent-deprecated-states

Conversation

@myii

@myii myii commented May 13, 2019

Copy link
Copy Markdown
Contributor
  • Route all to nginx/deprecated.sls

Cross-references:


Note: There is one common state before and after: nginx/init.sls. For that, I've checked for the presence of nginx:ng in the map instead.

Note: Implementation based on this PR.

* Route all to `nginx/deprecated.sls`
@daks

daks commented May 13, 2019

Copy link
Copy Markdown
Member

First: find a way to prevent the formula from running before having migrated settings is a great idea!

Second: you're only checking that nginx.common state is called to prevent this. I'm not sure this is sufficient, because people calling nginx directly will not be affected. Can we find other way to detect it: maybe with a specific pillar in nginx or detecting nginx.ng?

@myii

myii commented May 13, 2019

Copy link
Copy Markdown
Contributor Author

@daks Apologies, the implementation isn't clear because I started editing other lines as well to convert to {%-. In any case, you'll see that I'm specifically checking for nginx.ng when someone tries to run nginx. As for all of the other files, they are the entry points that existed before the change to v1.0.0, so they will be prevented from being run. Essentially, all bases are covered, except for someone trying to run the old nginx -- that will now trigger the new states and we can discuss (in a further PR) what to check in the pillar to confirm they are on the old style pillar. In the meantime, this PR will cover most of the potential problems, especially for nginx.ng. If there's no objection, I believe this should be merged ASAP to protect against unwanted breakages.

@n-rodriguez

Copy link
Copy Markdown
Contributor

looks good to me

@daks daks left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

OK for me. Can be merged quickly for me.

@myii

myii commented May 13, 2019

Copy link
Copy Markdown
Contributor Author

Thanks for the review, @n-rodriguez.

@myii

myii commented May 13, 2019

Copy link
Copy Markdown
Contributor Author

And thanks to you as well, @daks. Can either of you merge this please (CC: @n-rodriguez)?

@myii myii merged commit 46dff15 into saltstack-formulas:master May 13, 2019
@myii myii deleted the bug/prevent-deprecated-states branch May 13, 2019 12:06
@myii

myii commented May 13, 2019

Copy link
Copy Markdown
Contributor Author

Actually, since it's important and potentially time-critical, I've gone ahead and merged based on the the two approvals. Thanks again to everyone involved in reviewing and discussing these issues.

@saltstack-formulas-travis

Copy link
Copy Markdown

🎉 This PR is included in version 1.0.4 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@johnkeates

Copy link
Copy Markdown
Contributor

Thanks everyone, perhaps we can take this and note it down somewhere in the template formula docs? Haven't checked if we have a deprecation section in there yet, but we may want to.

@myii

myii commented May 13, 2019

Copy link
Copy Markdown
Contributor Author

@johnkeates Thanks for the suggestion, I've appended that to the documentation section in the "superlist" issue in the template-formula (link shown above).

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