Skip to content

Add puppetcore macos support #769

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Add puppetcore macos support #769

wants to merge 2 commits into from

Conversation

cthorn42
Copy link
Collaborator

@cthorn42 cthorn42 commented Apr 1, 2025

This is a work in progress, currently based on Josh's WIP: #766

@cthorn42 cthorn42 requested review from bastelfreak and a team as code owners April 1, 2025 21:30
cthorn42
cthorn42 commented Apr 1, 2025
View reviewed changes
manifests/osfamily/darwin.pp Outdated
@@ -20,12 +20,20 @@
} else {
$source = "puppet:///pe_packages/${pe_server_version}/${facts['platform_tag']}/${puppet_agent::package_name}-${puppet_agent::prepare::package_version}-1.osx${$productversion_major}.dmg"
}
} else {
} elsif $puppet_agent::collection and $puppet_agent::collection =~ /core/ {
if $puppet_agent::prepare::package_version =~ /^\d+\.\d+\.\d+\.\d+\.g([a-f0-9]+)+$/ {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is there a better way to determine if we need to set dev=true parameter?

Copy link
Contributor

Choose a reason for hiding this comment

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

This worked for me, splitting the version and checking for strictly more than 3 components:

    $dev = count(split($puppet_agent::prepare::package_version, '\.')) > 3

@cthorn42 cthorn42 force-pushed the add_puppetcore_macos_support branch from 191a2cc to 4a3c3e5 Compare April 2, 2025 15:46
joshcooper
joshcooper reviewed Apr 2, 2025
View reviewed changes
manifests/prepare/package.pp Outdated
mode => '0600',
}

$curl_command = "curl -1 -sL --netrc-file '${netrc_file}' -w '%{http_code}' -o '${local_package_file_path}' '${source}' > '${response_file}'"
Copy link
Contributor

Choose a reason for hiding this comment

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

suggest adding --fail so it returns non-zero exit on HTTP error

joshcooper
joshcooper reviewed Apr 2, 2025
View reviewed changes
manifests/prepare/package.pp
path => ['/usr/bin', '/bin'],
onlyif => "test -f '${netrc_file}'",
require => Exec['Download Puppet Agent for Darwin'],
}
Copy link
Contributor

Choose a reason for hiding this comment

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

may produce a "changed" event each time the agent runs?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't that the kind of issue https://forge.puppet.com/modules/puppetlabs/transition/readme exists for?

bastelfreak
bastelfreak reviewed Apr 2, 2025
View reviewed changes
manifests/prepare/package.pp Outdated
file { $netrc_file:
ensure => file,
content => "machine artifacts-puppetcore.puppet.com\nlogin ${download_username}\npassword ${download_password}\n",
mode => '0600',
Copy link
Collaborator

Choose a reason for hiding this comment

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

please add show_diff => false,, to ensure password won't be leaked

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

👍 Good catch.

@joshcooper joshcooper mentioned this pull request Apr 2, 2025
Merged
@cthorn42 cthorn42 force-pushed the add_puppetcore_macos_support branch from 4a3c3e5 to 2e64a41 Compare April 8, 2025 17:09
@joshcooper @klab-systems @cthorn42
Upgrade puppetcore* msi from artifacts-puppetcore.puppet.com
fd5342f 
When using the puppetcore collection on Windows, if we detect the installed
version does not match, then upgrade the MSI. Due to a puppet bug, we cannot
pass credentials in the `source` parameter. And `curl.exe` is not present in
our puppet-agent packages. So use powershell to download.

Co-authored-by: Kevin <114269618+klab-systems@users.noreply.github.com>
@cthorn42 cthorn42 force-pushed the add_puppetcore_macos_support branch 2 times, most recently from bcaf7d6 to 195c5e9 Compare April 22, 2025 18:59
bastelfreak
bastelfreak reviewed Apr 22, 2025
View reviewed changes
manifests/osfamily/darwin.pp Outdated
@@ -8,7 +8,7 @@
$productversion_array = split($facts['os']['macosx']['version']['major'], '[.]')
$productversion_major = $productversion_array[0]
}

$destination_name = undef
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do you set the variable here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The variable needs to get set for the scenario if puppetcore isn't being used. The way I had it set here is a work in progress, I'm about to push up changes that reflect how Josh fixed this with his Windows commit. I think it will be a bit cleaner in what is going on. Thanks for the comment I had forgotten to update this.

@cthorn42 cthorn42 force-pushed the add_puppetcore_macos_support branch from 195c5e9 to 96e3677 Compare April 22, 2025 20:44
bastelfreak
bastelfreak reviewed Apr 22, 2025
View reviewed changes
manifests/osfamily/darwin.pp Outdated
@@ -20,12 +19,25 @@
} else {
$source = "puppet:///pe_packages/${pe_server_version}/${facts['platform_tag']}/${puppet_agent::package_name}-${puppet_agent::prepare::package_version}-1.osx${$productversion_major}.dmg"
}
} elsif $puppet_agent::collection and $puppet_agent::collection =~ /core/ {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
} elsif $puppet_agent::collection and $puppet_agent::collection =~ /core/ {
} elsif $puppet_agent::collection =~ /core/ {

Copy link
Collaborator

Choose a reason for hiding this comment

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

$puppet_agent::collection cannot be undef, so we don't need to check if it's undef

bastelfreak
bastelfreak reviewed Apr 22, 2025
View reviewed changes
manifests/osfamily/darwin.pp Outdated
@@ -8,7 +8,6 @@
$productversion_array = split($facts['os']['macosx']['version']['major'], '[.]')
$productversion_major = $productversion_array[0]
}

if $puppet_agent::absolute_source {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this whole block becomes a bit more readable and shorter, if you do something like:

$source = if $puppet_agent::absolute_source {
...

Which enables you to get rid of all the following $source = in the logic block.

bastelfreak
bastelfreak reviewed Apr 22, 2025
View reviewed changes
manifests/prepare/package.pp Outdated
@@ -56,6 +56,45 @@
creates => $local_package_file_path,
require => File[$puppet_agent::params::local_packages_dir],
}
} elsif $puppet_agent::collection and $puppet_agent::collection =~ /core/ and $facts['os']['family'] =~ /Darwin/ {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
} elsif $puppet_agent::collection and $puppet_agent::collection =~ /core/ and $facts['os']['family'] =~ /Darwin/ {
} elsif $puppet_agent::collection =~ /core/ and $facts['os']['family'] =~ /Darwin/ {

@cthorn42 cthorn42 force-pushed the add_puppetcore_macos_support branch from 96e3677 to 4c2832f Compare April 23, 2025 21:47
joshcooper
joshcooper reviewed Apr 24, 2025
View reviewed changes
manifests/osfamily/darwin.pp
} else {
$source = "puppet:///pe_packages/${pe_server_version}/${facts['platform_tag']}/${puppet_agent::package_name}-${puppet_agent::prepare::package_version}-1.osx${$productversion_major}.dmg"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we want to keep puppet:///pe_packages/${pe_server_version}/.... as a valid source.

joshcooper
joshcooper reviewed Apr 24, 2025
View reviewed changes
tasks/install_shell.sh Outdated
if [[ "$collection" =~ core ]]; then
if [ -z "$password" ]; then
echo "A password parameter is required to install"
exit 1
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we check for an empty password earlier, is the if [ -z "$password ] check needed?

joshcooper
joshcooper reviewed Apr 24, 2025
View reviewed changes
tasks/install_shell.sh Outdated
@@ -431,6 +451,12 @@ do_curl() {
unable_to_retrieve_package
fi

grep "HTTP/2 401" $tmp_stderr 2>&1 >/dev/null
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know that we can safely assume the server replied with HTTP 2. Probably better to just check for:

Suggested change
grep "HTTP/2 401" $tmp_stderr 2>&1 >/dev/null
grep "401 Unauthorized" $tmp_stderr 2>&1 >/dev/null

@cthorn42
(maint) Update MacOS for puppetcore work
e6359ea 
This updates the download of puppet-agent when puppetcore packages are used.
The new 'puppetcore7' and 'puppetcore8' collections when used for MacOS will now
download puppetcore packages.
Due to a bug in Puppet for now we're going to depend on Curl to download the package.
@cthorn42 cthorn42 force-pushed the add_puppetcore_macos_support branch from 4c2832f to e6359ea Compare April 28, 2025 17:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bastelfreak bastelfreak bastelfreak left review comments

@joshcooper joshcooper joshcooper left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@cthorn42 @bastelfreak @joshcooper