Enable SaltBundle on SLES12-SP5 #22704

asmorodskyi · 2025-07-18T12:51:11Z

Ticket : https://progress.opensuse.org/issues/185413
VRs : create_HDD job - https://openqa.suse.de/tests/18476417
use of tweaked HDD : https://openqa.suse.de/tests/18476622

ricardobranco777 · 2025-07-18T12:59:13Z

tests/containers/bci_prepare.pm

@@ -38,7 +38,7 @@ sub packages_to_install {
    my $bci_virtualenv = get_var('BCI_VIRTUALENV', 0);

    # Avoid PackageKit to conflict about lock with zypper
-    script_run("timeout 20 pkcon quit") if (is_sle || is_opensuse);
+    script_run("timeout 20 pkcon quit") if (is_sle('>=15') || is_opensuse);


@volodymyrkatkalov had to enable this for SLES 12-SP5 on some module. Why remove it?

check create hdd VR

# timeout 20 pkcon quit; echo A_3TC-$?- timeout: failed to run command ‘pkcon’: No such file or directory

How about making that dynamic:

Suggested change

script_run("timeout 20 pkcon quit") if (is_sle('>=15') || is_opensuse);

script_run('command -v pkcon >/dev/null && timeout 20 pkcon quit');

Being the main purpose here to Avoid PackageKit to conflict about lock with zypper, no matter if it doesn't exist, we do want it just not running 😃

tests/containers/bci_prepare.pm

mloviska · 2025-07-18T13:03:00Z

tests/containers/bci_prepare.pm

@@ -150,7 +157,7 @@ sub run {
    my $branch = $bci_tests_branch ? "-b $bci_tests_branch" : '';
    script_run('rm -rf /root/BCI-tests');
    assert_script_run("git clone $branch -q --depth 1 $bci_tests_repo /root/BCI-tests");
-    assert_script_run('deactivate') if ($bci_virtualenv);
+    assert_script_run('deactivate') if ($bci_virtualenv || is_sle('=12-SP5'));


It would be better to set $bci_virtualenv in case of sle12sp5 as well. There is already a branch dedicated for this purpose in the code

this means that I would mean that I need to add if (is_sle('>=15')) into following places :
activate_virtual_env if ($bci_virtualenv); - because we don't need to bci/bin/activate virtualend
assert_script_run("pip --quiet install tox --ignore-installed six", timeout => 600); - because just running pip --quiet install tox
if (!grep(/-tox/, @packages)) { - we should somehow skip this check and just install tox

after realizing all this I decided that trying to do via already existing flow will create more mess than solve problems .
Another point is that from what I understand about https://progress.opensuse.org/issues/185413 idea is to drop python3.6 which means that for SLE 12 SP5 virtual env should be ONLY option so there is no point for a switcher which always in one position 🤷

My suggestion was about

if (is_sle('=12-SP5')){ assert_script_run('source /usr/lib/venv-salt-minion/bin/activate'); assert_script_run('pip --quiet install --upgrade pip', timeout => 600); assert_script_run('pip --quiet install tox', timeout => 600); $bci_virtualenv=1; }

The next usage of bci_virtualenv variable is just on line 160

okurz · 2025-07-18T13:55:41Z

tests/containers/bci_prepare.pm

@@ -38,7 +38,7 @@ sub packages_to_install {
    my $bci_virtualenv = get_var('BCI_VIRTUALENV', 0);

    # Avoid PackageKit to conflict about lock with zypper
-    script_run("timeout 20 pkcon quit") if (is_sle || is_opensuse);
+    script_run("timeout 20 pkcon quit") if (is_sle('>=15') || is_opensuse);


How about making that dynamic:

Suggested change

script_run("timeout 20 pkcon quit") if (is_sle('>=15') || is_opensuse);

script_run('command -v pkcon >/dev/null && timeout 20 pkcon quit');

tests/containers/bci_prepare.pm

okurz · 2025-07-18T13:56:35Z

tests/containers/bci_test.pm

@@ -137,6 +137,7 @@ sub run {
    reset_container_network_if_needed($engine);

    assert_script_run('source bci/bin/activate') if ($bci_virtualenv);
+    assert_script_run('source /usr/lib/venv-salt-minion/bin/activate') if (is_sle('=12-SP5'));


same question as above. And please use

Suggested change

assert_script_run('source /usr/lib/venv-salt-minion/bin/activate') if (is_sle('=12-SP5'));

assert_script_run('source /usr/lib/venv-salt-minion/bin/activate') if is_sle('=12-SP5');

why ? where it is defined ?

Just don't do the parentheses because that's the common style and that is also what you used in other places in this PR

assert_script_run('deactivate') if ($bci_virtualenv || is_sle('=12-SP5')); here I am using parentheses , should I remove them too ?

can you point me to the line where I haven't use them ? I don't see any

that's the common style

hard to follow this , unless it exists in written form which I can read . Another problem it should be accepted by maintainers of some certain repo . Otherwise we speaking about your personal preferences

Another problem it should be accepted by maintainers of some certain repo . Otherwise we speaking about your personal preferences

According to https://progress.opensuse.org/issues/101355 apparently it's far from clear who those "maintainers" would be. But according to https://github.com/os-autoinst/os-autoinst-distri-opensuse/graphs/contributors in 24 commits you should ask dzedro ask top contributor :)
However our coding styles link to https://github.com/os-autoinst/os-autoinst-distri-example/blob/main/tests/boot.pm which also uses the same style with return undef if match_has_tag 'no-boot-media';

For example in https://github.com/os-autoinst/os-autoinst-distri-opensuse/pull/22704/files#diff-ab51572902277af8fd2fd17d589a868d9196574c30014fc7fbe19f77abe259b1R155 the post-if is used without parentheses

you said "you using" . it is not me , I never do it like that because I find this form confusing . I am fine to follow same standard when :

it will be applied everywhere in one big PR

there will be common agreement in written form defining what is standard and also CI will check it

just pointing to one random line does not help here . I can give you contra examples in the same file

According to https://progress.opensuse.org/issues/101355 apparently it's far from clear who those "maintainers" would be. But according to https://github.com/os-autoinst/os-autoinst-distri-opensuse/graphs/contributors in 24 commits you should ask dzedro ask top contributor :)
However our coding styles link to https://github.com/os-autoinst/os-autoinst-distri-example/blob/main/tests/boot.pm which also uses the same style with return undef if match_has_tag 'no-boot-media';

sorry but all this for me is just your personal view on a problem which we have . And while I respect it but I leave option for myself to have MY view on that problem

Don't use whataboutism to push your personal preference. Be my guest to fix all non-compliant code locations in one big PR but it's not fair to expect that from a reviewer pointing out the inconsistency. I linked to a code example showing the preferred style. If you still think you want to do it differently then I will accept it for this PR but not be happy about it

Don't use whataboutism to push your personal preference. Be my guest to fix all non-compliant code locations in one big PR but it's not fair to expect that from a reviewer pointing out the inconsistency. I linked to a code example showing the preferred style. If you still think you want to do it differently then I will accept it for this PR but not be happy about it because I prefer to use a common code style assuming that the majority of code uses the same style as proposed by me and in https://github.com/os-autoinst/os-autoinst-distri-example/blob/main/tests/boot.pm . I could be wrong and it could be that many followed a different inconsistent style over the past years. In that case we should write the new style down as the preferred style at best into static code checks and if we can't automate it then at least in https://github.com/os-autoinst/os-autoinst-distri-opensuse/blob/master/CONTRIBUTING.md

okurz

The still open point would be https://github.com/os-autoinst/os-autoinst-distri-opensuse/pull/22704/files#r2216108721 which you disagree with. Besides that there are no other functional concerns.

asmorodskyi requested review from pdostal, grisu48, mloviska and ricardobranco777 as code owners July 18, 2025 12:51

ricardobranco777 reviewed Jul 18, 2025

View reviewed changes

mloviska reviewed Jul 18, 2025

View reviewed changes

asmorodskyi force-pushed the saltbundle branch from d129161 to e44935e Compare July 18, 2025 13:40

okurz requested changes Jul 18, 2025

View reviewed changes

Enable SaltBundle on SLES12-SP5

313ec5c

asmorodskyi force-pushed the saltbundle branch from e44935e to 313ec5c Compare July 18, 2025 14:19

okurz approved these changes Jul 21, 2025

View reviewed changes

	script_run("timeout 20 pkcon quit") if (is_sle('>=15') \|\| is_opensuse);
	script_run('command -v pkcon >/dev/null && timeout 20 pkcon quit');

	assert_script_run('source /usr/lib/venv-salt-minion/bin/activate') if (is_sle('=12-SP5'));
	assert_script_run('source /usr/lib/venv-salt-minion/bin/activate') if is_sle('=12-SP5');

Enable SaltBundle on SLES12-SP5 #22704

Are you sure you want to change the base?

Enable SaltBundle on SLES12-SP5 #22704

Conversation

asmorodskyi commented Jul 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mloviska Jul 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

okurz Jul 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

okurz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

asmorodskyi commented Jul 18, 2025 •

edited

Loading

mloviska Jul 18, 2025 •

edited

Loading

okurz Jul 20, 2025 •

edited

Loading