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.

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

Jump to bottom

Test for firewalld container in ALP product #18198

Merged
merged 2 commits into from
Nov 27, 2023
Merged

Test for firewalld container in ALP product #18198

merged 2 commits into from
Nov 27, 2023

Conversation

ilmanzo
Copy link
Contributor

@ilmanzo ilmanzo commented Nov 27, 2023 •
edited

Introduce a new multi-machine test for checking containerized firewalld workload in ALP product

@ilmanzo ilmanzo added the qe-core Tag used to filter PR's created by QE-Core's members or are assigned to them label Nov 27, 2023
ricardobranco777
ricardobranco777 approved these changes Nov 27, 2023
View reviewed changes
Copy link
Contributor

@ricardobranco777 ricardobranco777 left a comment

Choose a reason for hiding this comment

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

Nice work!

@clanig
Copy link
Contributor

clanig commented Nov 27, 2023

Nice PR, thank you.
Could you please outsource the exclusion of the loopback interface in the library in a separate commit? Other than that everything looks fine to me.
From my perspective the VRs all look the same.

@github-actions GitHub Actions
Copy link

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.

@mloviska
Copy link
Contributor

It is a pity that the test can be used only in qemu and also not sure about other archs. Other than that, it looks well! thanks

@foursixnine
Copy link
Member

It is a pity that the test can be used only in qemu and also not sure about other archs. Other than that, it looks well! thanks

doesn't sound like a bad idea to extend it :), but first lets unify the multiple network stacks, so they have similar test coverage of the features...

foursixnine
foursixnine approved these changes Nov 27, 2023
View reviewed changes
lib/mm_network.pm
@@ -128,7 +128,7 @@ sub configure_static_dns {
my $servers = join(" ", @{$conf->{nameserver}});

if ($is_nm) {
$nm_id = script_output('nmcli -t -f NAME c | head -n 1') unless ($nm_id);
$nm_id = script_output('nmcli -t -f NAME c | grep -v ^lo: | head -n 1') unless ($nm_id);
Copy link
Member

Choose a reason for hiding this comment

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

you could even do that on the perl side, but we can leave that for a follow up ticket, same for the ones earlier

@foursixnine
Copy link
Member

foursixnine commented Nov 27, 2023 •
edited

@ilmanzo feel free to merge :D, I'm not sure about the network connectivity part though that you mention on the ticket, but lets see

@ilmanzo ilmanzo merged commit 5af02ba into os-autoinst:master Nov 27, 2023
8 checks passed
@ilmanzo ilmanzo deleted the poo131189_ALP_firewalld branch November 27, 2023 12:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pdostal pdostal pdostal left review comments

@ricardobranco777 ricardobranco777 ricardobranco777 approved these changes

@foursixnine foursixnine foursixnine approved these changes

@dzedro dzedro dzedro approved these changes

@mloviska mloviska mloviska approved these changes

Assignees
No one assigned
Labels
qe-core Tag used to filter PR's created by QE-Core's members or are assigned to them
Projects
None yet
Milestone
No milestone
7 participants
@ilmanzo @clanig @mloviska @foursixnine @pdostal @dzedro @ricardobranco777