Skip to content

Conversation

@rominf
Copy link
Contributor

@rominf rominf commented Apr 2, 2019

Official bootstrap script written in Powershell is used.
CM_OPTIONS are passed as arguments (see README.md).

PS: the old way of installing Salt was limited and no longer works.

Official bootstrap script written in Powershell is used.
`CM_OPTIONS` are passed as arguments (see `README.md`).

PS: old way of installing Salt was limited and no longer works.
@arizvisa arizvisa self-requested a review December 25, 2019 07:58
@arizvisa
Copy link
Contributor

I'll look at getting this merged after the holidays when I have access to my saltstack boxes to test. That ok?

@arizvisa arizvisa added bug A bug in the way that a template is built or provisioned PR Priority (2) -- Important This PR fixes an issue in a particular component, or affects everything in a significant way labels Jan 3, 2020
Copy link
Contributor

@arizvisa arizvisa left a comment

Choose a reason for hiding this comment

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

Thanks for your PR. Other than the preservation of the CM_VERSION functionality, I think this PR is good-to-go.


@if errorlevel 1 echo ==^> WARNING: Error %ERRORLEVEL% was returned by: "%SALT_PATH%" /S %SALT_OPTIONS%
ver>nul
powershell "%SALT_PATH%" %CM_OPTIONS%
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we include the -version parameter here wth %CM_VERSION% so that we still expose that capability (to choose version) to users?

I think to avoid needing a conditional for the case of an undefined CM_VERSION, we can use "Latest" but I'll leave that up to you on whether or not you want to use a conditional for calling bootstrap-salt with an undefined CM_VERSION, or a conditional to set CM_VERSION to Latest if it's undefined.

You can also specify a variable `CM_VERSION`, if supported by the
configuration management tool, to override the default of `latest`.
You can also specify a variable `CM_VERSION` for all configuration management
tools except Salt, to override the default of `latest`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please see the comment in the review for script/cmtool.bat. I think we might be able to still preserve the functionality of allowing the user to set the salt version via CM_VERSION.

@arizvisa
Copy link
Contributor

arizvisa commented Jan 3, 2020

Hey @rominf, let me know if you don't have time to incorporate these changes and I'll work on integrating them into your PR. Now that I can help maintain this repo, PRs like this that fix busted functionality are high on my priority list.

Thanks for your patience, good sir!

@rominf
Copy link
Contributor Author

rominf commented Jan 4, 2020

Hey @arizvisa, thanks for asking about the time. Indeed, I'm a little bit busy right now. Feel free to do whatever you want with that PR :)

@arizvisa arizvisa self-assigned this Jan 5, 2020
@arizvisa
Copy link
Contributor

arizvisa commented Jan 5, 2020

Ok. I'll see what I can do.

arizvisa added a commit to arizvisa/boxcutter-windows that referenced this pull request Jan 6, 2020
arizvisa added a commit to arizvisa/boxcutter-windows that referenced this pull request Jan 6, 2020
…which retains usability of the CM_VERSION variable when installing salt.
@arizvisa arizvisa added the PR Superseded This particular PR has been superseded by a different PR mentioned in the discussion label Jan 6, 2020
@arizvisa
Copy link
Contributor

arizvisa commented Jan 6, 2020

I noticed that each of the configuration management tools have a way of passing options to them (CHEF_OPTIONS, PUPPET_OPTIONS, etc.), so I ended up extending your CM_OPTIONS enhancement that you added for Salt to all of them.

I also added the changes to explicitly specify the SaltStack version since you can't ever be sure if a current version of SaltStack is a broken release or not.

These have been consolidated into PR #201. As a result I'm marking this as superseded and will close this PR once #201 gets merged. Thanks for your contribution good sir!

arizvisa added a commit to arizvisa/boxcutter-windows that referenced this pull request Jan 9, 2020
arizvisa added a commit to arizvisa/boxcutter-windows that referenced this pull request Jan 9, 2020
…which retains usability of the CM_VERSION variable when installing salt.
arizvisa added a commit that referenced this pull request Jan 9, 2020
…ains usability of the CM_VERSION variable when installing salt.
@arizvisa
Copy link
Contributor

arizvisa commented Jan 9, 2020

Ok. PR #201 with your changes has been merged. Thanks for your contribution!

@arizvisa arizvisa closed this Jan 9, 2020
daxgames pushed a commit to daxgames/boxcutter_windows that referenced this pull request Feb 26, 2020
daxgames pushed a commit to daxgames/boxcutter_windows that referenced this pull request Feb 26, 2020
…which retains usability of the CM_VERSION variable when installing salt.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug A bug in the way that a template is built or provisioned PR Priority (2) -- Important This PR fixes an issue in a particular component, or affects everything in a significant way PR Superseded This particular PR has been superseded by a different PR mentioned in the discussion

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants