From dcf9ac6b2fcdb283b6ca4f5ff0b895628ecc9f0e Mon Sep 17 00:00:00 2001 From: david22swan Date: Fri, 19 May 2023 10:24:58 +0100 Subject: [PATCH 1/3] (GH-585/CONT-998) Fix for safe_directory logic Due to mistake in the logic unsafe directory was previously removed on every other run. The logic previously checked whether the unsafe directory needed to be added, removing it if this was false without taking into account when it was already set but we wanted it to be left in place. --- lib/puppet/provider/vcsrepo/git.rb | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/lib/puppet/provider/vcsrepo/git.rb b/lib/puppet/provider/vcsrepo/git.rb index 6a87ce91..dbf27254 100644 --- a/lib/puppet/provider/vcsrepo/git.rb +++ b/lib/puppet/provider/vcsrepo/git.rb @@ -609,7 +609,7 @@ def update_safe_directory if should_add_safe_directory? add_safe_directory - else + elsif should_remove_safe_directory? remove_safe_directory end end @@ -637,6 +637,12 @@ def should_add_safe_directory? !safe_directories.include?(@resource.value(:path)) # directory should not already be in the list end + # @!visibility private + def should_remove_safe_directory? + !@resource.value(:safe_directory) && # safe_directory should be false + safe_directories.include?(@resource.value(:path)) # directory should be in the list + end + # @!visibility private def git_remote_action(*args) proxy = @resource.value(:http_proxy) From 04d5e68c695ca1633c231c61f58e59752df1b355 Mon Sep 17 00:00:00 2001 From: david22swan Date: Fri, 19 May 2023 14:25:12 +0100 Subject: [PATCH 2/3] Update test to check for safe directory --- spec/acceptance/clone_repo_spec.rb | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/spec/acceptance/clone_repo_spec.rb b/spec/acceptance/clone_repo_spec.rb index 696d83f2..7f120546 100644 --- a/spec/acceptance/clone_repo_spec.rb +++ b/spec/acceptance/clone_repo_spec.rb @@ -238,6 +238,12 @@ it { is_expected.to be_directory } it { is_expected.to be_owned_by 'vagrant' } end + + describe file('~/.gitconfig') do + subject { super().content } + + it { is_expected.to match %r{directory = /tmp/vcsrepo/testrepo_owner} } + end end context 'with with a group' do From 64586c3ab5d55e1218cc5d2040a00acd600b0768 Mon Sep 17 00:00:00 2001 From: david22swan Date: Fri, 19 May 2023 16:22:57 +0100 Subject: [PATCH 3/3] (GH-585/CONT-998) Update safe directory to be set at the system level Safe directory is now set at a system wide level rather than at a global user level in order to ensure it is as effective as possible. --- README.md | 2 ++ lib/puppet/provider/vcsrepo/git.rb | 6 +++--- spec/acceptance/clone_repo_spec.rb | 2 +- spec/unit/puppet/provider/vcsrepo/git_spec.rb | 2 +- 4 files changed, 7 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index ad38e235..3f2d1761 100644 --- a/README.md +++ b/README.md @@ -864,6 +864,8 @@ For example, setting the `owner` parameter on a resource would cause Puppet runs Impacted users are now advised to use the new `safe_directory` parameter on Git resources. Explicitily setting the value to `true` will add the current path specified on the resource to the `safe.directory` git configuration for the current user (global scope) allowing the Puppet run to continue without error. +Safe directory configuration will be stored within the system wide configuration file `/etc/gitconfig`. + ## Development diff --git a/lib/puppet/provider/vcsrepo/git.rb b/lib/puppet/provider/vcsrepo/git.rb index dbf27254..81da6073 100644 --- a/lib/puppet/provider/vcsrepo/git.rb +++ b/lib/puppet/provider/vcsrepo/git.rb @@ -591,7 +591,7 @@ def git_version # @!visibility private def safe_directories - args = ['config', '--global', '--get-all', 'safe.directory'] + args = ['config', '--system', '--get-all', 'safe.directory'] begin d = git_with_identity(*args) || '' d.split('\n') @@ -617,7 +617,7 @@ def update_safe_directory # @!visibility private def add_safe_directory notice("Adding '#{@resource.value(:path)}' to safe directory list") - args = ['config', '--global', '--add', 'safe.directory', @resource.value(:path)] + args = ['config', '--system', '--add', 'safe.directory', @resource.value(:path)] git_with_identity(*args) end @@ -626,7 +626,7 @@ def remove_safe_directory return unless safe_directories.include?(@resource.value(:path)) notice("Removing '#{@resource.value(:path)}' from safe directory list") - args = ['config', '--global', '--unset', 'safe.directory', @resource.value(:path)] + args = ['config', '--system', '--unset', 'safe.directory', @resource.value(:path)] git_with_identity(*args) end diff --git a/spec/acceptance/clone_repo_spec.rb b/spec/acceptance/clone_repo_spec.rb index 7f120546..a63b860e 100644 --- a/spec/acceptance/clone_repo_spec.rb +++ b/spec/acceptance/clone_repo_spec.rb @@ -239,7 +239,7 @@ it { is_expected.to be_owned_by 'vagrant' } end - describe file('~/.gitconfig') do + describe file('/etc/gitconfig') do subject { super().content } it { is_expected.to match %r{directory = /tmp/vcsrepo/testrepo_owner} } diff --git a/spec/unit/puppet/provider/vcsrepo/git_spec.rb b/spec/unit/puppet/provider/vcsrepo/git_spec.rb index e8ad4931..b3aa42f9 100644 --- a/spec/unit/puppet/provider/vcsrepo/git_spec.rb +++ b/spec/unit/puppet/provider/vcsrepo/git_spec.rb @@ -253,7 +253,7 @@ def branch_a_list(include_branch = nil?) expect(provider).to receive(:path_exists?).and_return(true) expect(provider).to receive(:path_empty?).and_return(false) provider.destroy - expect(provider).to receive(:exec_git).with('config', '--global', '--get-all', 'safe.directory') + expect(provider).to receive(:exec_git).with('config', '--system', '--get-all', 'safe.directory') expect(provider).to receive(:exec_git).with('clone', resource.value(:source), resource.value(:path)) expect(provider).to receive(:update_submodules) expect(provider).to receive(:update_remote_url).with('origin', resource.value(:source)).and_return false