-
Notifications
You must be signed in to change notification settings - Fork 420
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
refactor(salt-minion): remove old code, rearrange & reformat #416
base: master
Are you sure you want to change the base?
Conversation
@myii this should be thoroughly tested on windows....and macos |
@aboe76 That's a great idea and I've been asking around for how to get this done. Do you know the best way for this? |
Some feedback on Slack about MacOS:
|
MacOS (and Windows) also supported by Cirrus CI, which is currently being evaluated in saltstack-formulas/template-formula#118. |
When can we get chance to have this merged ? |
@sticky-note We need to get some testing done on MacOS and Windows; I don't have those available. Maybe @noelmcloughlin can help with MacOS? Not sure who has access to Windows minions at this current time. I did have a nice large pool of Windows minions, not so long ago. |
79bc1c6
to
07d3113
Compare
OK, I've rebased this to remove the conflict so this is ready for testing again. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good, I see no issues, except for testing...
Well, this formula doesn't appear to be working too well on MacOS anyway (ref: #352, with a recent update from @noelmcloughlin). So just need to get this code tested on Windows. |
Could this be merged so I can fix (expected) conflicts in #426 |
@noelmcloughlin I would like to but this still hasn't been tested for breakages on Windows minions. I used to have access to a whole bunch of them but not at the current time. Unless we can find someone to do that. Any suggestions? Or should we ask in the Windows room in Slack? |
MacOS issues can be tracked in #352 - problems remain. |
@@ -50,11 +50,17 @@ salt-minion: | |||
- name: {{ salt_settings.salt_minion }} | |||
{%- if salt_settings.version %} | |||
- version: {{ salt_settings.version }} | |||
{%- endif %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I resolved the conflict but missed this - line 53 needs to be deleted.
As discussed on Slack: https://freenode.logbot.info/saltstack-formulas/20190514#c2178003.
This is only the first stage. I believe this should be followed up by splitting this into multiple files. As pointed out in the discussion: