From c2f1864d58c5aedcf489a8f9c8201d64435e6ea6 Mon Sep 17 00:00:00 2001 From: "robert.richardson" Date: Fri, 16 Aug 2024 15:30:50 +0200 Subject: [PATCH] Refactor git functions into git module - Make path variable and ssh batch mode usage method parameters - Fix merge conflicts after rebase to master branch - Avoid array recreation with _run_cmd helper method - Minor fixes --- lib/OpenQA/Git.pm | 114 ++++++++++++++++++++++++++--------- lib/OpenQA/Task/Git/Clone.pm | 56 +++-------------- t/14-grutasks.t | 39 ++++++------ t/16-utils-runcmd.t | 5 +- 4 files changed, 118 insertions(+), 96 deletions(-) diff --git a/lib/OpenQA/Git.pm b/lib/OpenQA/Git.pm index bfde432176c..c700cec7e4f 100644 --- a/lib/OpenQA/Git.pm +++ b/lib/OpenQA/Git.pm @@ -4,6 +4,7 @@ package OpenQA::Git; use Mojo::Base -base, -signatures; +use Mojo::Util 'trim'; use Cwd 'abs_path'; use OpenQA::Utils qw(run_cmd_with_log_return_error); @@ -21,45 +22,67 @@ sub config ($self, $args = undef) { return $app->config->{'scm git'}; } -sub _prepare_git_command ($self, $args = undef) { - my $dir = $args->{dir} // $self->dir; - if ($dir !~ /^\//) { - my $absolute_path = abs_path($dir); - $dir = $absolute_path if ($absolute_path); +sub _validate_attributes ($self) { + for my $mandatory_property (qw(app dir user)) { + die "no $mandatory_property specified" unless $self->$mandatory_property(); } - return ('git', '-C', $dir); } -sub _format_git_error ($result, $error_message) { - if ($result->{stderr} or $result->{stdout}) { - $error_message .= ': ' . $result->{stdout} . $result->{stderr}; +sub _run_cmd ($self, $args, $options = {}) { + my $include_git_path = $options->{include_git_path} // 1; + my $ssh_batchmode = $options->{ssh_batchmode} // 0; + my @cmd; + + if ($ssh_batchmode) { + push @cmd, 'env', 'GIT_SSH_COMMAND="ssh -oBatchMode=yes"'; } - return $error_message; + + push @cmd, $self->_prepare_git_command($include_git_path), @$args; + + my $result = run_cmd_with_log_return_error(\@cmd); + if (!$result->{status}) { + $self->app->log->error("Git command failed: @cmd - Error: $result->{stderr}"); + } + return $result; } -sub _validate_attributes ($self) { - for my $mandatory_property (qw(app dir user)) { - die "no $mandatory_property specified" unless $self->$mandatory_property(); +sub _prepare_git_command ($self, $include_git_path) { + if ($include_git_path) { + my $dir = $self->dir; + die 'no valid directory was found during git preparation' unless $dir; + if ($dir !~ /^\//) { + my $absolute_path = abs_path($dir); + $dir = $absolute_path if ($absolute_path); + } + return ('git', '-C', $dir); + } + else { + return 'git'; } } +sub _format_git_error ($self, $result, $error_message) { + my $dir = $self->dir; + if ($result->{stderr} or $result->{stdout}) { + $error_message .= " ($dir): " . $result->{stdout} . $result->{stderr}; + } + return $error_message; +} + sub set_to_latest_master ($self, $args = undef) { $self->_validate_attributes; - - my @git = $self->_prepare_git_command($args); - if (my $update_remote = $self->config->{update_remote}) { - my $res = run_cmd_with_log_return_error([@git, 'remote', 'update', $update_remote]); - return _format_git_error($res, 'Unable to fetch from origin master') unless $res->{status}; + my $res = $self->_run_cmd(['remote', 'update', $update_remote]); + return $self->_format_git_error($res, 'Unable to fetch from origin master') unless $res->{status}; } if (my $update_branch = $self->config->{update_branch}) { if ($self->config->{do_cleanup} eq 'yes') { - my $res = run_cmd_with_log_return_error([@git, 'reset', '--hard', 'HEAD']); - return _format_git_error($res, 'Unable to reset repository to HEAD') unless $res->{status}; + my $res = $self->_run_cmd(['reset', '--hard', 'HEAD']); + return $self->_format_git_error($res, 'Unable to reset repository to HEAD') unless $res->{status}; } - my $res = run_cmd_with_log_return_error([@git, 'rebase', $update_branch]); - return _format_git_error($res, 'Unable to reset repository to origin/master') unless $res->{status}; + my $res = $self->_run_cmd(['rebase', $update_branch]); + return $self->_format_git_error($res, 'Unable to reset repository to origin/master') unless $res->{status}; } return undef; @@ -68,30 +91,63 @@ sub set_to_latest_master ($self, $args = undef) { sub commit ($self, $args = undef) { $self->_validate_attributes; - my @git = $self->_prepare_git_command($args); my @files; # stage changes for my $cmd (qw(add rm)) { next unless $args->{$cmd}; push(@files, @{$args->{$cmd}}); - my $res = run_cmd_with_log_return_error([@git, $cmd, @{$args->{$cmd}}]); - return _format_git_error($res, "Unable to $cmd via Git") unless $res->{status}; + my $res = $self->_run_cmd([$cmd, @{$args->{$cmd}}]); + return $self->_format_git_error($res, "Unable to $cmd via Git") unless $res->{status}; } # commit changes my $message = $args->{message}; my $author = sprintf('--author=%s <%s>', $self->user->fullname, $self->user->email); - my $res = run_cmd_with_log_return_error([@git, 'commit', '-q', '-m', $message, $author, @files]); - return _format_git_error($res, 'Unable to commit via Git') unless $res->{status}; + my $res = $self->_run_cmd(['commit', '-q', '-m', $message, $author, @files]); + return $self->_format_git_error($res, 'Unable to commit via Git') unless $res->{status}; # push changes if (($self->config->{do_push} || '') eq 'yes') { - $res = run_cmd_with_log_return_error([@git, 'push']); - return _format_git_error($res, 'Unable to push Git commit') unless $res->{status}; + $res = $self->_run_cmd(['push']); + return $self->_format_git_error($res, 'Unable to push Git commit') unless $res->{status}; } return undef; } +sub get_current_branch ($self) { + my $r = $self->_run_cmd(['branch', '--show-current']); + die $self->_format_git_error($r, 'Error detecting current branch') unless $r->{status}; + return trim($r->{stdout}); +} + +sub get_remote_default_branch ($self, $url) { + my $r = $self->_run_cmd(['ls-remote', '--symref', $url, 'HEAD'], {include_git_path => 0, ssh_batchmode => 1}); + die "Error detecting remote default branch name for '$url': $r->{stdout} $r->{stderr}" + unless $r->{status} && $r->{stdout} =~ m{refs/heads/(\S+)\s+HEAD}; + return $1; +} + +sub clone_url ($self, $url) { + my $r = $self->_run_cmd(['clone', $url, $self->dir], {include_git_path => 0, ssh_batchmode => 1}); + die $self->_format_git_error($r, "Failed to clone $url") unless $r->{status}; +} + +sub get_origin_url ($self) { + my $r = $self->_run_cmd(['remote', 'get-url', 'origin']); + die $self->_format_git_error($r, 'Failed to get origin url') unless $r->{status}; + return trim($r->{stdout}); +} + +sub fetch ($self, $branch_arg) { + my $r = $self->_run_cmd(['fetch', 'origin', $branch_arg], {ssh_batchmode => 1}); + die $self->_format_git_error($r, "Failed to fetch from '$branch_arg'") unless $r->{status}; +} + +sub reset_hard ($self, $branch) { + my $r = $self->_run_cmd(['reset', '--hard', "origin/$branch"]); + die $self->_format_git_error($r, "Failed to reset to 'origin/$branch'") unless $r->{status}; +} + 1; diff --git a/lib/OpenQA/Task/Git/Clone.pm b/lib/OpenQA/Task/Git/Clone.pm index e6025a448a2..f57a68fdeea 100644 --- a/lib/OpenQA/Task/Git/Clone.pm +++ b/lib/OpenQA/Task/Git/Clone.pm @@ -13,7 +13,6 @@ sub register ($self, $app, @) { $app->minion->add_task(git_clone => \&_git_clone_all); } - # $clones is a hashref with paths as keys and urls to git repos as values. # The urls may also refer to a branch via the url fragment. # If no branch is set, the default branch of the remote (if target path doesn't exist yet) @@ -45,7 +44,7 @@ sub _git_clone_all ($job, $clones) { for my $path (sort { length($a) <=> length($b) } keys %$clones) { my $url = $clones->{$path}; die "Don't even think about putting '..' into '$path'." if $path =~ /\.\./; - eval { _git_clone($job, $ctx, $path, $url) }; + eval { _git_clone($app, $job, $ctx, $path, $url) }; next unless my $error = $@; my $max_retries = $ENV{OPENQA_GIT_CLONE_RETRIES} // 10; return $job->retry($retry_delay) if $job->retries < $max_retries; @@ -53,69 +52,32 @@ sub _git_clone_all ($job, $clones) { } } -sub _get_current_branch ($path) { - my $r = run_cmd_with_log_return_error(['git', '-C', $path, 'branch', '--show-current']); - die "Error detecting current branch for '$path': $r->{stderr}" unless $r->{status}; - return trim($r->{stdout}); -} - -sub _ssh_git_cmd ($git_args) { - return ['env', 'GIT_SSH_COMMAND="ssh -oBatchMode=yes"', 'git', @$git_args]; -} - -sub _get_remote_default_branch ($url) { - my $r = run_cmd_with_log_return_error(_ssh_git_cmd(['ls-remote', '--symref', $url, 'HEAD'])); - die "Error detecting remote default branch name for '$url': $r->{stdout} $r->{stderr}" - unless $r->{status} && $r->{stdout} =~ m{refs/heads/(\S+)\s+HEAD}; - return $1; -} - -sub _git_clone_url_to_path ($url, $path) { - my $r = run_cmd_with_log_return_error(_ssh_git_cmd(['clone', $url, $path])); - die "Failed to clone $url into '$path': $r->{stderr}" unless $r->{status}; -} - -sub _git_get_origin_url ($path) { - my $r = run_cmd_with_log_return_error(['git', '-C', $path, 'remote', 'get-url', 'origin']); - die "Failed to get origin url for '$path': $r->{stderr}" unless $r->{status}; - return trim($r->{stdout}); -} - -sub _git_fetch ($path, $branch_arg) { - my $r = run_cmd_with_log_return_error(_ssh_git_cmd(['-C', $path, 'fetch', 'origin', $branch_arg])); - die "Failed to fetch from '$branch_arg': $r->{stderr}" unless $r->{status}; -} - -sub _git_reset_hard ($path, $branch) { - my $r = run_cmd_with_log_return_error(['git', '-C', $path, 'reset', '--hard', "origin/$branch"]); - die "Failed to reset to 'origin/$branch': $r->{stderr}" unless $r->{status}; -} - -sub _git_clone ($job, $ctx, $path, $url) { +sub _git_clone ($app, $job, $ctx, $path, $url) { + my $git = OpenQA::Git->new(app => $app, dir => $path); $ctx->debug(qq{Updating $path to $url}); $url = Mojo::URL->new($url); my $requested_branch = $url->fragment; $url->fragment(undef); # An initial clone fetches all refs, we are done - return _git_clone_url_to_path($url, $path) unless -d $path; + return $git->clone_url($url) unless -d $path; - my $origin_url = _git_get_origin_url($path); + my $origin_url = $git->get_origin_url; if ($url ne $origin_url) { $ctx->warn("Local checkout at $path has origin $origin_url but requesting to clone from $url"); return; } unless ($requested_branch) { - my $remote_default = _get_remote_default_branch($url); + my $remote_default = $git->get_remote_default_branch($url); $requested_branch = $remote_default; $ctx->debug(qq{Remote default branch $remote_default}); } - my $current_branch = _get_current_branch($path); + my $current_branch = $git->get_current_branch; # updating default branch (including checkout) - _git_fetch($path, $requested_branch); - _git_reset_hard($path, $requested_branch) if ($requested_branch eq $current_branch); + $git->fetch($requested_branch); + $git->reset_hard($requested_branch) if ($requested_branch eq $current_branch); } 1; diff --git a/t/14-grutasks.t b/t/14-grutasks.t index 0b8d679e3c1..b18daea2723 100644 --- a/t/14-grutasks.t +++ b/t/14-grutasks.t @@ -645,14 +645,14 @@ subtest 'handling dying GRU task' => sub { }; subtest 'git clone' => sub { - my $openqa_utils = Test::MockModule->new('OpenQA::Task::Git::Clone'); + my $openqa_git = Test::MockModule->new('OpenQA::Git'); my @mocked_git_calls; my $clone_dirs = { '/etc/' => 'http://localhost/foo.git', '/root/' => 'http://localhost/foo.git#foobranch', '/this_directory_does_not_exist/' => 'http://localhost/bar.git', }; - $openqa_utils->redefine( + $openqa_git->redefine( run_cmd_with_log_return_error => sub ($cmd) { push @mocked_git_calls, "@$cmd"; my $stdout = ''; @@ -675,21 +675,21 @@ subtest 'git clone' => sub { is $res->{result}, 'Job successfully executed', 'minion job result indicates success'; #<<< no perltidy my $expected_calls = [ - # /etc/ - ['get-url' => 'git -C /etc/ remote get-url origin'], - ['default remote' => 'env GIT_SSH_COMMAND="ssh -oBatchMode=yes" git ls-remote --symref http://localhost/foo.git HEAD'], - ['current branch' => 'git -C /etc/ branch --show-current'], - ['fetch default' => 'env GIT_SSH_COMMAND="ssh -oBatchMode=yes" git -C /etc/ fetch origin master'], - ['reset' => 'git -C /etc/ reset --hard origin/master'], - - # /root - ['get-url' => 'git -C /root/ remote get-url origin'], - ['current branch' => 'git -C /root/ branch --show-current'], - ['fetch branch' => 'env GIT_SSH_COMMAND="ssh -oBatchMode=yes" git -C /root/ fetch origin foobranch'], - - # /this_directory_does_not_exist/ - ['clone' => 'env GIT_SSH_COMMAND="ssh -oBatchMode=yes" git clone http://localhost/bar.git /this_directory_does_not_exist/'], - ]; + # /etc/ + ['get-url' => 'git -C /etc/ remote get-url origin'], + ['default remote' => 'env GIT_SSH_COMMAND="ssh -oBatchMode=yes" git ls-remote --symref http://localhost/foo.git HEAD'], + ['current branch' => 'git -C /etc/ branch --show-current'], + ['fetch default' => 'env GIT_SSH_COMMAND="ssh -oBatchMode=yes" git -C /etc/ fetch origin master'], + ['reset' => 'git -C /etc/ reset --hard origin/master'], + + # /root + ['get-url' => 'git -C /root/ remote get-url origin'], + ['current branch' => 'git -C /root/ branch --show-current'], + ['fetch branch' => 'env GIT_SSH_COMMAND="ssh -oBatchMode=yes" git -C /root/ fetch origin foobranch'], + + # /this_directory_does_not_exist/ + ['clone' => 'env GIT_SSH_COMMAND="ssh -oBatchMode=yes" git clone http://localhost/bar.git /this_directory_does_not_exist/'], +]; #>>> no perltidy for my $i (0 .. $#$expected_calls) { my $test = $expected_calls->[$i]; @@ -706,13 +706,16 @@ subtest 'git clone' => sub { subtest 'git clone retried on failure' => sub { $ENV{OPENQA_GIT_CLONE_RETRIES} = 1; - $openqa_utils->redefine(_git_clone => sub (@) { die "fake error\n" }); + my $openqa_clone = Test::MockModule->new('OpenQA::Task::Git::Clone'); + $openqa_clone->redefine(_git_clone => sub (@) { die "fake error\n" }); $res = run_gru_job($t->app, 'git_clone', $clone_dirs, {priority => 10}); is $res->{retries}, 1, 'job retries incremented'; is $res->{state}, 'inactive', 'job set back to inactive'; }; subtest 'git clone fails when all retry attempts exhausted' => sub { $ENV{OPENQA_GIT_CLONE_RETRIES} = 0; + my $openqa_clone = Test::MockModule->new('OpenQA::Task::Git::Clone'); + $openqa_clone->redefine(_git_clone => sub (@) { die "fake error\n" }); $res = run_gru_job($t->app, 'git_clone', $clone_dirs, {priority => 10}); is $res->{retries}, 0, 'job retries not incremented'; is $res->{state}, 'failed', 'job considered failed'; diff --git a/t/16-utils-runcmd.t b/t/16-utils-runcmd.t index 2a5f8075843..3420498458b 100644 --- a/t/16-utils-runcmd.t +++ b/t/16-utils-runcmd.t @@ -53,7 +53,8 @@ subtest 'make git commit (error handling)' => sub { my $res; stdout_like { $res = $git->commit({cmd => 'status', message => 'test'}) } qr/.*\[warn\].*fatal: Not a git repository/i, 'git message found'; - like $res, qr'^Unable to commit via Git: fatal: (N|n)ot a git repository \(or any', 'Git error message returned'; + like $res, qr"^Unable to commit via Git \($empty_tmp_dir\): fatal: (N|n)ot a git repository \(or any", + 'Git error message returned'; }; # setup mocking @@ -117,7 +118,7 @@ subtest 'git commands with mocked run_cmd_with_log_return_error' => sub { $mock_return_value{stdout} = ''; is( $git->set_to_latest_master, - 'Unable to fetch from origin master: mocked error', + 'Unable to fetch from origin master (foo/bar): mocked error', 'an error occurred on remote update' ); is_deeply(\@executed_commands, [[qw(git -C foo/bar remote update origin)],], 'git reset not attempted',)