Skip to content
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

Removing duplicated save log output from ssh #21019

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

hadeskun
Copy link
Contributor

Removing duplicated save log output -E option from ssh

Copy link

Great PR! Please pay attention to the following items before merging:

Files matching lib/publiccloud/**.pm:

  • Provide VRs for both QE-C as well as QE-SAP (check Confluence for more info)

This is an automatically generated QA checklist based on modified files.

@hadeskun hadeskun force-pushed the sshverbose branch 3 times, most recently from 0ec8715 to 5b5306d Compare January 22, 2025 11:51
@@ -148,7 +148,14 @@ sub _prepare_ssh_cmd {
$cmd = "\$'$cmd'";
}

my $ssh_cmd = sprintf('ssh -E /var/tmp/ssh_sut.log %s "%s@%s" -- %s', $args{ssh_opts}, $args{username}, $self->public_ip, $cmd);
my $ssh_cmd = '';
Copy link
Contributor

Choose a reason for hiding this comment

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

my $ssh_cmd;
my $ssh_log_file;

$ssh_log_file = "-E /var/tmp/ssh_sut.log"  if ($args{ssh_opts} !~ /-E \/var\/tmp\/ssh_sut\.log/);
$ssh_cmd = sprintf('ssh %s %s "%s@%s" -- %s', $ssh_log_file, $args{ssh_opts}, $args{username}, $self->public_ip, $cmd);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

@@ -148,7 +148,14 @@ sub _prepare_ssh_cmd {
$cmd = "\$'$cmd'";
}

my $ssh_cmd = sprintf('ssh -E /var/tmp/ssh_sut.log %s "%s@%s" -- %s', $args{ssh_opts}, $args{username}, $self->public_ip, $cmd);
my $ssh_cmd = '';
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to initialize the value

Suggested change
my $ssh_cmd = '';
my $ssh_cmd;

@hadeskun hadeskun force-pushed the sshverbose branch 2 times, most recently from 462c98e to f1789d1 Compare January 22, 2025 12:59
Copy link
Member

@pdostal pdostal left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@mpagot mpagot left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines 151 to 153
my $ssh_cmd;
my $ssh_log_file;
$ssh_log_file = "-E /var/tmp/ssh_sut.log" if ($args{ssh_opts} !~ /-E \/var\/tmp\/ssh_sut\.log/);
Copy link
Contributor

@m-dati m-dati Jan 22, 2025

Choose a reason for hiding this comment

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

Kind suggestion, if you like it:

Suggested change
my $ssh_cmd;
my $ssh_log_file;
$ssh_log_file = "-E /var/tmp/ssh_sut.log" if ($args{ssh_opts} !~ /-E \/var\/tmp\/ssh_sut\.log/);
my $ssh_cmd;
my $log = '/var/tmp/ssh_sut.log';
my $ssh_log_file;
$ssh_log_file = "-E $log" if ($args{ssh_opts} !~ m{-E\s+$log});

Copy link
Contributor

@m-dati m-dati Jan 22, 2025

Choose a reason for hiding this comment

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

in perl regexpr we can use different separator than '/', to avoid escaping it. Also, here we consider possible more spaces inbetween.

Copy link
Contributor

@m-dati m-dati Jan 22, 2025

Choose a reason for hiding this comment

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

Also, suggested a more VR as negative-test, pre-adding the log in ssh_opts, to see that it still works.

Copy link
Contributor

@alvarocarvajald alvarocarvajald Jan 22, 2025

Choose a reason for hiding this comment

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

If the log name is in $log, I'd say it's better for readability to do the regexp as:

    $ssh_log_file = "-E $log" if ($args{ssh_opts} !~ m/-E\s+$log/);

It's more common.

OTOH, if the log name is not in a variable as in the original code, then better to use a different separator for the regexp to avoid escaping the / characters:

    $ssh_log_file = '-E /var/tmp/ssh_sut.log' if ($args{ssh_opts} !~ m|-E /var/tmp/ssh_sut\.log|);

Copy link
Contributor

Choose a reason for hiding this comment

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

And wdyt about shorter form in #21019 (comment) ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Fewer lines are always good IMO ;)

Comment on lines 152 to 154
my $ssh_log_file;
$ssh_log_file = "-E /var/tmp/ssh_sut.log" if ($args{ssh_opts} !~ /-E \/var\/tmp\/ssh_sut\.log/);
$ssh_cmd = sprintf('ssh %s %s "%s@%s" -- %s', $ssh_log_file, $args{ssh_opts}, $args{username}, $self->public_ip, $cmd);
Copy link
Contributor

Choose a reason for hiding this comment

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

Or a more compact suggestion:

Suggested change
my $ssh_log_file;
$ssh_log_file = "-E /var/tmp/ssh_sut.log" if ($args{ssh_opts} !~ /-E \/var\/tmp\/ssh_sut\.log/);
$ssh_cmd = sprintf('ssh %s %s "%s@%s" -- %s', $ssh_log_file, $args{ssh_opts}, $args{username}, $self->public_ip, $cmd);
my $log = '/var/tmp/ssh_sut.log';
$ssh_cmd = sprintf('ssh %s %s "%s@%s" -- %s', (($args{ssh_opts} !~ m{-E\s+$log}) ? "-E $log" : ''), $args{ssh_opts}, $args{username}, $self->public_ip, $cmd);

Copy link
Contributor

@alvarocarvajald alvarocarvajald left a comment

Choose a reason for hiding this comment

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

LGTM, although I also suggest not using // in the regexp in L153 to avoid escaping the / characters as suggested by @m-dati

Removing duplicated save log output for ssh command.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants