Skip to content

Enable SaltBundle on SLES12-SP5 #22704

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.

Sign up for GitHub

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

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

Enable SaltBundle on SLES12-SP5 #22704

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 16 additions & 8 deletions tests/containers/bci_prepare.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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('command -v pkcon >/dev/null && timeout 20 pkcon quit');

# common packages
my @packages = ('git-core', 'python3', 'jq');
Expand All @@ -53,20 +53,20 @@ sub packages_to_install {
push @packages, ('python3-virtualenv') if ($bci_virtualenv);
} elsif ($host_distri eq 'sles' || $host_distri =~ /leap/) {
my $version = "$version.$sp";
push @packages, ('python3-virtualenv') if ($bci_virtualenv);
if ($version =~ /12\./) {
script_retry("SUSEConnect --auto-agree-with-licenses -p sle-sdk/$version/$arch", delay => 60, retry => 3, timeout => $scc_timeout);
# PackageHub is needed for jq
script_retry("SUSEConnect -p PackageHub/12.5/$arch", delay => 60, retry => 3, timeout => $scc_timeout);
script_retry('zypper -n in jq', retry => 3);
# Note tox is not available on SLES12
push @packages, qw(python36-pip);
die "virtualenv is not supported on 12-SP5" if ($bci_virtualenv);
assert_script_run("zypper ar -f http://download.suse.de/ibs/SUSE:/SLE-12:/Update:/Products:/SaltBundle:/Update/standard/ saltbundle");
assert_script_run("zypper -n install venv-salt-minion");
} elsif ($version =~ /15\.[1-3]/) {
push @packages, ('python3-virtualenv') if ($bci_virtualenv);
push @packages, ('skopeo');
} else {
script_retry("SUSEConnect -p sle-module-python3/$version/$arch", delay => 60, retry => 3, timeout => $scc_timeout) if ($host_distri =~ /sles/);
push @packages, qw(python311 skopeo python311-pip python311-tox);
push @packages, ('python3-virtualenv') if ($bci_virtualenv);
}
} elsif ($host_distri =~ /opensuse/) {
push @packages, qw(skopeo python3-pip python3-tox);
Expand Down Expand Up @@ -129,10 +129,18 @@ sub run {
foreach my $pkg (@packages) {
zypper_call("--quiet in $pkg", timeout => 300);
}
activate_virtual_env if ($bci_virtualenv);
if (!grep(/-tox/, @packages)) {
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 --ignore-installed six", timeout => 600);
assert_script_run('pip --quiet install tox', timeout => 600);
$bci_virtualenv = 1;
}
else {
activate_virtual_env if ($bci_virtualenv);
if (!grep(/-tox/, @packages)) {
assert_script_run('pip --quiet install --upgrade pip', timeout => 600);
assert_script_run("pip --quiet install tox --ignore-installed six", timeout => 600);
}
}
} else {
die "Unexpected distribution ($host_distri) has been used";
Expand Down
3 changes: 2 additions & 1 deletion tests/containers/bci_test.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'));
Copy link
Member

Choose a reason for hiding this comment

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

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');

Copy link
Member Author

Choose a reason for hiding this comment

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

why ? where it is defined ?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

@okurz okurz Jul 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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';

Copy link
Member Author

Choose a reason for hiding this comment

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

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 :

  1. it will be applied everywhere in one big PR
  2. 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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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


record_info('Run', "Starting the tests for the following environments:\n$test_envs");
assert_script_run("cd /root/BCI-tests && git fetch && git reset --hard $bci_tests_branch");
Expand All @@ -158,7 +159,7 @@ sub run {
$self->run_tox_cmd($env);
}

assert_script_run('deactivate') if ($bci_virtualenv);
assert_script_run('deactivate') if ($bci_virtualenv || is_sle('=12-SP5'));

# Mark the job as failed if any of the tests failed
die("$error_count tests failed.") if ($error_count > 0);
Expand Down