-
Notifications
You must be signed in to change notification settings - Fork 2
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
Add support for provisioning macOS jenkins agents #38
base: latest
Are you sure you want to change the base?
Conversation
# Install java | ||
remote_file "/tmp/jdk8.pkg" do | ||
source "https://github.com/adoptium/temurin8-binaries/releases/download/jdk8u362-b09/OpenJDK8U-jdk_x64_mac_hotspot_8u362b09.pkg" | ||
not_if "pkgutil --pkg-info net.temurin.8.jdk" | ||
end | ||
|
||
execute "install java" do | ||
command "installer -pkg /tmp/jdk8.pkg -target /" | ||
not_if "pkgutil --pkg-info net.temurin.8.jdk" | ||
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.
I've opted not to prematurely add logic to support JDK8 vs JDK11, which is an upcoming transition required on the build farm. I have chosen to keep this PR straightforward and add the JDK11 support in a follow-up.
I see that
|
923e18a
to
0b92f31
Compare
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.
Sorry for not reviewing it before. Mostly good to go, one suggestion I think that it is important about security and password.
end | ||
|
||
directory "/Users/jenkins/Library/LaunchAgents" do | ||
owner "jenkins" |
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.
Nitpick: we could abstract the user and group in the attributes file and use the variable in this file replacing the name in multiple places. Feel free to skip at this point, not sure if it worth it.
-url #{node['osrfbuild']['agent']['jenkins_url']} | ||
-name #{agent_name} | ||
-username #{jenkins_agent_user['username']} | ||
-password #{jenkins_agent_user['password']} |
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.
Not familiar with Mac, but using the password
argument directly here could imply a security problem if the process if visible for all users in the machine. I remember to have worked on this after a security review point that out in our infra. The linux recipe uses --passwordFile
modified in #17
-deleteExistingClients | ||
-labels #{labels.join(' ')} | ||
-e HOMEWBREW_FORCE_VENDOR_RUBY=1 | ||
-e MAKE_JOBS=8 |
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.
Nitpick: we don't have a better way for defining this but maybe moving the same hardcoded value to an attribute can make people life easier if they need to modify it.
-password #{jenkins_agent_user['password']} | ||
-description #{description} | ||
-mode exclusive | ||
-executors 1 |
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.
Nitpick: also a good candidate to be included in an attribute file.
Base level macOS setup is too trial-and-error and will have to be done by hand for now.
macOS does not create a group for each user.
Anticipating future Apple Silicon macs.
We want homebrew operations to run via the Jenkins user to keep permissions in good order but the Chef executor cannot do so without a password prompt which fails. So this is just easier. The biggest downside is that it does not monitor/maintain updates but neither does homebrew-cask by default either so 🤷.
And --env nor -env seem to work.
There was friction in automating these steps with chef which we haven't worked through.
0b92f31
to
9ee2d25
Compare
|
||
# Verify SSH and VNC remote access are enabled, which should already true for | ||
# our hosted machines. | ||
# Verify remote management is enabled _only_ for administrator |
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.
# Verify remote management is enabled _only_ for administrator |
This can lead to VNC connection problems
This PR adds a new recipe for creating macOS agents.
Right now it's actually quite basic because the low-level macOS host setup is hostile to automation and has drifted from previous methods to set things up via
defaults write
. The machines that this was developed on and shipped for were provisioned by hand using a privately documented process.I do not think that the low level setup is required for this PR to have value so while I hope that we'll find ways to maintainably audit the initial machine setup that lack does not block this PR from review and merge.