Wrapper for zypper_call and trup_call #18204

dzedro · 2023-11-27T12:57:39Z

Related ticket: https://progress.opensuse.org/issues/117028
Verification run:
https://dzedro.suse.cz/tests/301#step/update_kernel/106 SLE Micro
https://dzedro.suse.cz/tests/302#step/update_kernel/26 SLES
https://openqa.suse.de/tests/overview?distri=sle&version=15-SP5&build=jpupava_wrapper

czerw · 2023-11-28T06:51:08Z

I found, not kernel related test, which is great candidate for this PR: https://github.com/os-autoinst/os-autoinst-distri-opensuse/blob/master/tests/console/year_2038_detection.pm

github-actions · 2023-11-28T08:40:10Z

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

Files matching lib/**.pm:

Consider adding or extending unit tests in t/

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

dzedro · 2023-11-28T08:47:37Z

I found, not kernel related test, which is great candidate for this PR: https://github.com/os-autoinst/os-autoinst-distri-opensuse/blob/master/tests/console/year_2038_detection.pm

Thank you @czerw, the year_2038_detection test is only on 15-SP5, if you want me to do it on other versions or do another test, let me please know. I think there should be no differences between versions in this case.

tests/console/year_2038_detection.pm

czerw

LGTM, it's good start to have things more seamless.

mdoucha · 2023-11-28T10:44:38Z

lib/wrapper.pm

+    my $trup_packages = $args{trup_packages} // $args{packages};
+    my $zypper_packages = $args{zypper_packages} // $args{packages};
+    die 'At least one of packages, trup_packages, zypper_packages arguments is required' unless defined($args{packages} || $args{trup_packages} || $args{zypper_packages});
+    my $timeout = $args{timeout} // 500;


First, why add a new file? This should go to utils.pm or at least qam.pm because it'll be used a lot.

Second, I really don't like that all packages have to be passed in a keyword argument. I'd prefer the package list handling like this:

my ($packages, %args) = @_; if (is_transactional) { $packages .= ' ' . $args{trup_extra} // ''; } else { $packages .= ' ' . $args{zypper_extra} // ''; }

So that I can call just install_package('foo');.

I think I read somewhere it should be in separated package, but I don't mind to have it in likely utils.pm.
Sure, I will do the packages default parameter without keyword argument.

I changed the code, if nobody disagrees @czerw @foursixnine ? I would move the install_package into utils.

This should go to utils.pm or at least qam.pm because it'll be used a lot.

That it will be used a lot isn't a good reason to keep making lib/utils grow, so @dzedro lets keep it separate.

In the end, Jozef's work would end up as a stepping stone to move towards something that abstracts most things from the caller, and end up in $self->action($packages) or something like that

OK, can we at least rename the file so that it sounds like something related to package installation? rpm.pm, package_utils.pm, or something like that...

lib/wrapper.pm

foursixnine · 2023-11-29T06:46:28Z

lib/wrapper.pm

+# Maintainer: QE Core <qe-core@suse.de>
+
+package wrapper;
+use Mojo::Base qw(Exporter);


In the future, we can take a look into using perl signatures https://www.perl.com/article/announcing-perl-7/

foursixnine · 2023-11-29T06:51:13Z

lib/wrapper.pm

+        record_info('install_package', $args{skip_trup}) if $args{skip_trup} =~ /\w+/;
+        return if $args{skip_trup};


Suggested change

record_info('install_package', $args{skip_trup}) if $args{skip_trup} =~ /\w+/;

return if $args{skip_trup};

if $args{skip_trup}{ record_info "Skipping changes in transactional system as requested"; return };

record_info('install_package', $args{skip_trup}) if $args{skip_trup} =~ /\w+/;

I can put also all the code on one line, can I ? 🙄

I can put also all the code on one line, can I ?

Well I mean to have a clear indicator that there's an intended skip

I would do it, but then record_info after return is pointless.
I did it just because existing code had record_info, there is option to leave message,
must be not done always.

record_info('install_package', ...); would be just pointless spam...

foursixnine · 2023-11-29T06:51:25Z

lib/wrapper.pm

+        record_info('install_package', $args{skip_zypper}) if $args{skip_zypper} =~ /\w+/;
+        return if $args{skip_zypper};


Same as above

foursixnine

Looking good @dzedro, I guess the update or patch are what come next, isn't it?

(random question, is there a difference between patch vs dup/up? from a customer point of view?

dzedro · 2023-11-29T09:35:29Z

Looking good @dzedro, I guess the update or patch are what come next, isn't it?

(random question, is there a difference between patch vs dup/up? from a customer point of view?

IDK, Whatever is needed
I guess path and up are kind of same, patch just does first update zypper then in next run other packages,
up does all at onde, dup is like up with vendor change. There is probably more differences, this is what I know.

Regarding location of install_package, I will not put it into utils, because of failure https://dzedro.suse.cz/tests/252, probably some circular dependency.
So please agree where you want it, I don't mind wrapper, qam, utils or other package.

mdoucha · 2023-11-29T10:31:09Z

Regarding location of install_package, I will not put it into utils, because of failure https://dzedro.suse.cz/tests/252, probably some circular dependency. So please agree where you want it, I don't mind wrapper, qam, utils or other package.

qam.pm is fine.

dzedro · 2023-11-29T10:58:39Z

It's in qam.pm, I had ready also package_utils.pm

lib/qam.pm

https://progress.opensuse.org/issues/117028

mdoucha

LGTM

dzedro requested a review from czerw November 27, 2023 12:57

dzedro force-pushed the install_pkg branch from 235eb83 to 8092bea Compare November 27, 2023 13:20

os-autoinst deleted a comment from github-actions bot Nov 27, 2023

czerw requested a review from mdoucha November 28, 2023 06:51

dzedro force-pushed the install_pkg branch from 8092bea to c3af497 Compare November 28, 2023 08:39

czerw reviewed Nov 28, 2023

View reviewed changes

tests/console/year_2038_detection.pm Outdated Show resolved Hide resolved

dzedro force-pushed the install_pkg branch 2 times, most recently from d6c0b62 to c5f922e Compare November 28, 2023 09:15

czerw approved these changes Nov 28, 2023

View reviewed changes

mdoucha requested changes Nov 28, 2023

View reviewed changes

mdoucha reviewed Nov 28, 2023

View reviewed changes

lib/wrapper.pm Outdated Show resolved Hide resolved

dzedro force-pushed the install_pkg branch from c5f922e to 1784c12 Compare November 28, 2023 13:37

foursixnine reviewed Nov 29, 2023

View reviewed changes

foursixnine approved these changes Nov 29, 2023

View reviewed changes

dzedro force-pushed the install_pkg branch from 1784c12 to d926ace Compare November 29, 2023 09:35

dzedro force-pushed the install_pkg branch from d926ace to fafa82a Compare November 29, 2023 10:41

dzedro marked this pull request as ready for review November 29, 2023 10:55

foursixnine reviewed Nov 29, 2023

View reviewed changes

lib/qam.pm Outdated Show resolved Hide resolved

Wrapper for zypper_call and trup_call

472cf0f

https://progress.opensuse.org/issues/117028

dzedro force-pushed the install_pkg branch from fafa82a to 472cf0f Compare November 30, 2023 06:03

dzedro requested a review from mdoucha November 30, 2023 06:08

foursixnine approved these changes Nov 30, 2023

View reviewed changes

mdoucha approved these changes Dec 4, 2023

View reviewed changes

czerw merged commit 49164a8 into os-autoinst:master Dec 4, 2023
8 checks passed

dzedro deleted the install_pkg branch December 5, 2023 08:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Wrapper for zypper_call and trup_call #18204

Wrapper for zypper_call and trup_call #18204

dzedro commented Nov 27, 2023 •

edited

czerw commented Nov 28, 2023

github-actions bot commented Nov 28, 2023

dzedro commented Nov 28, 2023

czerw left a comment

mdoucha Nov 28, 2023

dzedro Nov 28, 2023

dzedro Nov 28, 2023

foursixnine Nov 29, 2023 •

edited

mdoucha Nov 29, 2023

foursixnine Nov 29, 2023

dzedro Nov 29, 2023

foursixnine Nov 29, 2023

dzedro Nov 29, 2023

foursixnine Nov 29, 2023

dzedro Nov 29, 2023

mdoucha Nov 29, 2023

foursixnine Nov 29, 2023

foursixnine left a comment

dzedro commented Nov 29, 2023

mdoucha commented Nov 29, 2023

dzedro commented Nov 29, 2023 •

edited

mdoucha left a comment

		record_info('install_package', $args{skip_trup}) if $args{skip_trup} =~ /\w+/;
		return if $args{skip_trup};

		record_info('install_package', $args{skip_zypper}) if $args{skip_zypper} =~ /\w+/;
		return if $args{skip_zypper};

Wrapper for zypper_call and trup_call #18204

Wrapper for zypper_call and trup_call #18204

Conversation

dzedro commented Nov 27, 2023 • edited

czerw commented Nov 28, 2023

github-actions bot commented Nov 28, 2023

dzedro commented Nov 28, 2023

czerw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

foursixnine Nov 29, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

foursixnine left a comment

Choose a reason for hiding this comment

dzedro commented Nov 29, 2023

mdoucha commented Nov 29, 2023

dzedro commented Nov 29, 2023 • edited

mdoucha left a comment

Choose a reason for hiding this comment

dzedro commented Nov 27, 2023 •

edited

foursixnine Nov 29, 2023 •

edited

dzedro commented Nov 29, 2023 •

edited