-
-
Notifications
You must be signed in to change notification settings - Fork 10.7k
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
salt: 3006.5 (new cask) #162371
salt: 3006.5 (new cask) #162371
Conversation
This cask is a replacement for the deprecated and disabled |
I would like to know @SMillerDev's opinion about this. |
Sounds fine to me, it's been deprecated and disabled for a while already. |
Casks/s/salt.rb
Outdated
postflight do | ||
random_str = (0...8).map { rand(65..90).chr }.join | ||
%w[api master minion syndic].each do |daemon| | ||
plist_file = "/Library/LaunchDaemons/com.saltstack.salt.#{daemon}.plist" | ||
xml, = system_command! "plutil", | ||
args: ["-convert", "xml1", "-o", "-", "--", plist_file], | ||
sudo: true | ||
xml = Plist.parse_xml(xml) | ||
|
||
xml["EnvironmentVariables"] = {} unless xml.key?("EnvironmentVariables") | ||
xml["EnvironmentVariables"]["HOMEBREW_PREFIX"] = HOMEBREW_PREFIX.to_s | ||
|
||
path = xml["EnvironmentVariables"]["PATH"] || "/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin" | ||
path = "#{HOMEBREW_PREFIX}/bin:#{path}" unless path.split(":").include?("#{HOMEBREW_PREFIX}/bin") | ||
xml["EnvironmentVariables"]["PATH"] = path | ||
|
||
new_plist_file = "/tmp/#{random_str}.#{File.basename(plist_file)}" | ||
File.write(new_plist_file, xml.to_plist) | ||
system_command! "plutil", | ||
args: ["-lint", new_plist_file] | ||
|
||
system_command! "mv", | ||
args: [new_plist_file, plist_file], | ||
sudo: true | ||
system_command! "chown", | ||
args: ["root:wheel", plist_file], | ||
sudo: true | ||
end | ||
end |
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.
Can you provide some commentary on why this postflight
block is necessary, and perhaps add a comment to code explaining it succinctly?
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.
Of course!
The postflight
is intended to add the HOMEBREW_PREFIX
to the environment in order to make salt
fully compatible with homebrew. Some modules require this environment variable to find not linked libraries. So, considering that this cask is being installed using homebrew, it is a valid assumption that we want that salt can find homebrew libraries.
This is an example of salt looking for HOMEBREW_PREFIX
env variable. Also, in this PR, the use of this variable is added: saltstack/salt#64924
Apart of HOMEBREW_PREFIX
, the PATH
variable is also updated (only if needed) to include homebrew binaries.
I can add a comment to the postflight
code explaining this if you think it would be useful!
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 don't think we should use a post block to fix the deficiencies in salt.
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.
So, as an alternative, could this be handled as an optional step, disabled by default?
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.
No, it should be fixed upstream
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.
OK... My last idea is to add some information in the caveats. So people can manually add these variables if needed. What do you think?
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.
Maybe make an issue/PR upstream and link there with a caveat?
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'll try to improve this PR: saltstack/salt#64924
So, I think neither the modification of the plist files, nor the caveats documentation will be necessary.
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.
postflight
removed
db71ecd
to
9ee2b06
Compare
Casks/s/salt.rb
Outdated
|
||
Load services you need: | ||
|
||
sudo launchctl load -w /Library/LaunchDaemons/com.saltstack.salt.api |
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.
Shouldn't this be a .plist
?
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.
Done!
Casks/s/salt.rb
Outdated
After installation, configure the Salt minion ID, and the Salt master location, | ||
replacing the placeholder text with custom information: | ||
|
||
sudo salt-config -i [MINION_ID] -m [SALT_MASTER_HOST] | ||
|
||
See Configure the Salt master and minions for more configuration options: | ||
https://docs.saltproject.io/salt/install-guide/en/latest/topics/configure-master-minion.html |
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.
Since this is not a homebrew specific instruction I don't think it should be a caveat
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.
Done!
9ee2b06
to
cf48511
Compare
Casks/s/salt.rb
Outdated
regex(/salt-(\d+(?:\.\d+)?(?:-\d+)?)-py3-#{arch}\.pkg/) | ||
end | ||
|
||
conflicts_with formula: "salt" |
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.
Now that salt
has been removed from homebrew-core
, this line needs to be removed here too.
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.
Done!
Casks/s/salt.rb
Outdated
After installation, configure the Salt minion ID, and the Salt master location, | ||
replacing the placeholder text with custom information: | ||
|
||
sudo salt-config -i [MINION_ID] -m [SALT_MASTER_HOST] | ||
|
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.
Since this is not specific to the Homebrew install.
After installation, configure the Salt minion ID, and the Salt master location, | |
replacing the placeholder text with custom information: | |
sudo salt-config -i [MINION_ID] -m [SALT_MASTER_HOST] |
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.
Done!
Casks/s/salt.rb
Outdated
|
||
sudo salt-config -i [MINION_ID] -m [SALT_MASTER_HOST] | ||
|
||
Load services you need: |
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.
Load services you need: | |
Included services: |
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.
Done!
cf48511
to
ab69a64
Compare
Casks/s/salt.rb
Outdated
|
||
url "https://repo.saltproject.io/salt/py3/macos/minor/#{version}/salt-#{version}-py3-#{arch}.pkg" | ||
name "Salt" | ||
desc "World's fastest, most intelligent and scalable automation engine" |
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.
desc "World's fastest, most intelligent and scalable automation engine" | |
desc "Automation and infrastructure management engine" |
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.
Done!
Casks/s/salt.rb
Outdated
zap trash: [ | ||
"/etc/salt", | ||
] |
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.
zap trash: [ | |
"/etc/salt", | |
] | |
zap trash: "/etc/salt" |
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.
Done!
Casks/s/salt.rb
Outdated
|
||
livecheck do | ||
url "https://repo.saltproject.io/salt/py3/macos/latest" | ||
regex(/salt-(\d+(?:\.\d+)?(?:-\d+)?)-py3-#{arch}\.pkg/) |
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.
regex(/salt-(\d+(?:\.\d+)?(?:-\d+)?)-py3-#{arch}\.pkg/) | |
regex(/salt[._-]v?(\d+(?:\.\d+)+)-py3-#{arch}\.pkg/) |
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.
Done!
ab69a64
to
9f030a2
Compare
Thanks @cdalvaro Using |
Important: Do not tick a checkbox if you haven’t performed its action. Honesty is indispensable for a smooth review process.
After making any changes to a cask, existing or new, verify:
brew audit --cask --online salt
is error-free.brew style --fix salt
reports no offenses.Additionally, if adding a new cask:
brew audit --cask --new salt
worked successfully.salt
. Although that formula is deprecated and this cask is intended to replace it.brew install --cask salt
worked successfully.brew uninstall --cask salt
worked successfully.