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

adding salt-minion restart option from official FAQ page #325

Merged
merged 1 commit into from
Aug 8, 2017

Conversation

mechleg
Copy link
Contributor

@mechleg mechleg commented Jul 27, 2017

I have run into an issue similar to those described in #136 . I was hoping to find a different solution then the one recently merged, mostly to not have another package dependency.

I found such a solution here, using the "bg" option in cmd.run:
https://docs.saltstack.com/en/latest/faq.html#what-is-the-best-way-to-restart-a-salt-minion-daemon-using-salt-after-upgrade

This has been working on all my Linux minions better then the salt-minion restart method used in the current formula. It should also work on Windows though I am not able to test that.

This PR changes the current default salt-minion restart behavior to that described in the linked FAQ. It should be backward compatible and add similar functionality to salt versions without the "bg" option.

Thoughts? Good idea? Bad idea?

@aboe76 aboe76 requested a review from iggy July 27, 2017 20:46
@aboe76
Copy link
Member

aboe76 commented Jul 27, 2017

@tlemarchand could you take a look to?

@javierbertoli
Copy link
Member

It LGTM, although I have no Windows box to test the Windows parts.

@aboe76
Copy link
Member

aboe76 commented Aug 4, 2017

@mechleg can you confirm this works on windows? @javierbertoli and I don't have a setup with windows minions.

@mechleg
Copy link
Contributor Author

mechleg commented Aug 5, 2017

Unfortunately, neither do I, Linux only shop over here.

I figured since it was on the FAQ, someone tested those commands in Windows at some point. If no one uses this formula with Windows, I could take those bits out.

@aboe76
Copy link
Member

aboe76 commented Aug 5, 2017

@mechleg and @javierbertoli because this is behind a switch I think it's safe to merge and if some windows minion user is complaining we can fix it then.

@mechleg
Copy link
Contributor Author

mechleg commented Aug 8, 2017

Sounds good to me, anything else I need to do?

@aboe76
Copy link
Member

aboe76 commented Aug 8, 2017

@mechleg no, just need a confirmation from @javierbertoli then I merged it.

@javierbertoli
Copy link
Member

@aboe76, I think it's ok to merge.

@aboe76 aboe76 merged commit 8ad2bb6 into saltstack-formulas:master Aug 8, 2017
@aboe76
Copy link
Member

aboe76 commented Aug 8, 2017

@mechleg and @javierbertoli thanks for this. hopefully it will help others.

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