Consolidate e2e resource creation by mkolesnik · Pull Request #931 · istio-ecosystem/sail-operator

mkolesnik · 2025-06-19T08:24:06Z

What type of PR is this?

What this PR does / why we need it:

Istio and IstioCNI creation is done in multiple places in the E2E tests, and is very repetitive.
This PR consolidates most og it, making the tests less repetitive and more maintainable.

Which issue(s) this PR fixes:

Fixes #

Related Issue/PR #

Additional information:

codecov · 2025-06-19T08:28:31Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 76.85%. Comparing base (5569498) to head (7eaf9e8).
Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #931   +/-   ##
=======================================
  Coverage   76.85%   76.85%           
=======================================
  Files          44       44           
  Lines        2683     2683           
=======================================
  Hits         2062     2062           
  Misses        526      526           
  Partials       95       95

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

MaxBab

Very nice.

fjglira

only two small comments, for the rest LGTM. I think is a great improve

fjglira · 2025-06-19T10:32:27Z

tests/e2e/util/common/e2e_utils.go

+func indent(str string) string {
+	indent := strings.Repeat(" ", 2)
+	return indent + strings.ReplaceAll(str, "\n", "\n"+indent)
+}


Small comment only here, we already have this same function declared here and being used in the same file. Can you please make it available in some way to use it in both places

fjglira · 2025-06-19T10:34:52Z

tests/e2e/util/common/e2e_utils.go

+func withClusterName(m string, k kubectl.Kubectl) string {
+	if k.ClusterName == "" {
+		return m
+	}
+
+	return m + " on " + k.ClusterName
+}


kubectl util already has a definition for this here it can be confusing in my opinion

That one is a public function on the Kubectl type, whereas this one is private and won't be used outside the package.
I think it should be fine but if you think differently let me know and I'll change it

This code is very repetitive and would better live in a common function. Signed-off-by: Mike Kolesnik <mkolesni@redhat.com>

fjglira

lgtm

* upstream/main: Consolidate e2e resource creation (istio-ecosystem#931)

* Consolidate IstioCNI creation in e2e tests This code is very repetitive and would better live in a common function. Signed-off-by: Mike Kolesnik <mkolesni@redhat.com> * Consolidate Istio creation in e2e tests This code is very repetitive and would better live in a common function. Signed-off-by: Mike Kolesnik <mkolesni@redhat.com> --------- Signed-off-by: Mike Kolesnik <mkolesni@redhat.com>

* Consolidate multicluster operator deploy/cleanup (#902) Now that we have `Cleaner` clean up after each test, it makes sense to run this logic once in the suite, rather than after each test (which cleans up its own resources). Signed-off-by: Mike Kolesnik <mkolesni@redhat.com> * Fix typo in the ReportAfterSuite text (#911) Signed-off-by: Sridhar Gaddam <sgaddam@redhat.com> * Fix helm issue during e2e test run operator installation (#914) * Fix helm issue during e2e test run operator installation Signed-off-by: Francisco H <frherrer@redhat.com> Fix command error Signed-off-by: Francisco H <frherrer@redhat.com> * Apply suggestions from code review Co-authored-by: Mike Kolesnik <mkolesni@redhat.com> Signed-off-by: Francisco H <frherrer@redhat.com> * Improves from code review Signed-off-by: Francisco H <frherrer@redhat.com> * Update comment in build and push script Signed-off-by: Francisco H <frherrer@redhat.com> --------- Signed-off-by: Francisco H <frherrer@redhat.com> Co-authored-by: Mike Kolesnik <mkolesni@redhat.com> * Consolidate e2e resource creation (#931) * Consolidate IstioCNI creation in e2e tests This code is very repetitive and would better live in a common function. Signed-off-by: Mike Kolesnik <mkolesni@redhat.com> * Consolidate Istio creation in e2e tests This code is very repetitive and would better live in a common function. Signed-off-by: Mike Kolesnik <mkolesni@redhat.com> --------- Signed-off-by: Mike Kolesnik <mkolesni@redhat.com> * Use `Eventually` in remote secret generation (#921) In this test the secret generation sometimes is attempted before everything is ready, leading to flaky, hard to debug, failures. Using `Eventually` to generate the secret avoids these failures. Fixes: #866 Signed-off-by: Mike Kolesnik <mkolesni@redhat.com> --------- Signed-off-by: Mike Kolesnik <mkolesni@redhat.com> Signed-off-by: Sridhar Gaddam <sgaddam@redhat.com> Signed-off-by: Francisco H <frherrer@redhat.com> Co-authored-by: Mike Kolesnik <mkolesni@redhat.com> Co-authored-by: Sridhar Gaddam <sgaddam@redhat.com>

* Stabilize e2e dual-stack test (istio-ecosystem#936) (istio-ecosystem#939) Signed-off-by: Mikhail Abramov <mabramov@redhat.com> * fix IstioRevisionTag uninstall when revision namespace changes (istio-ecosystem#945) Fixes istio-ecosystem#917. This is quite a complex edge case, I'll do my best to explain. If you look at the integration test I wrote (part of this commit), you can follow along. When a tag was created that referenced a revision in ns1, then patched to reference a revision in ns2, we failed to clean up the old helm install in ns1. That almost never causes problems - except in the case where afterwards, you remove all tags, and then create a new one with the same name that is never reconciled. If that tag is deleted, the operator will attempt to uninstall that lingering helm release, which fails, leading the operator to deadlock, and stopping the user from deleting the tag. The only way to get the operator back into a working state would be to manually delete that lingering helm release. That is a pretty significant bug in my opinion. Signed-off-by: Daniel Grimm <dgrimm@redhat.com> Co-authored-by: Daniel Grimm <dgrimm@redhat.com> * Expose "Profile" parameter in UI for Istio and IstioCNI (istio-ecosystem#924) (istio-ecosystem#947) During deployment of Istio in ambient mode, the user should be able to set the "profile: ambient" for Istio and IstioCNI resources. Currently, this parameter is hidden from the UI. Turn on the visibility of this parameter in the UI. Signed-off-by: Maxim Babushkin <mbabushk@redhat.com> * [Manual Cherry pick] Pick e2e improve and solving helm issues (istio-ecosystem#948) * Consolidate multicluster operator deploy/cleanup (istio-ecosystem#902) Now that we have `Cleaner` clean up after each test, it makes sense to run this logic once in the suite, rather than after each test (which cleans up its own resources). Signed-off-by: Mike Kolesnik <mkolesni@redhat.com> * Fix typo in the ReportAfterSuite text (istio-ecosystem#911) Signed-off-by: Sridhar Gaddam <sgaddam@redhat.com> * Fix helm issue during e2e test run operator installation (istio-ecosystem#914) * Fix helm issue during e2e test run operator installation Signed-off-by: Francisco H <frherrer@redhat.com> Fix command error Signed-off-by: Francisco H <frherrer@redhat.com> * Apply suggestions from code review Co-authored-by: Mike Kolesnik <mkolesni@redhat.com> Signed-off-by: Francisco H <frherrer@redhat.com> * Improves from code review Signed-off-by: Francisco H <frherrer@redhat.com> * Update comment in build and push script Signed-off-by: Francisco H <frherrer@redhat.com> --------- Signed-off-by: Francisco H <frherrer@redhat.com> Co-authored-by: Mike Kolesnik <mkolesni@redhat.com> * Consolidate e2e resource creation (istio-ecosystem#931) * Consolidate IstioCNI creation in e2e tests This code is very repetitive and would better live in a common function. Signed-off-by: Mike Kolesnik <mkolesni@redhat.com> * Consolidate Istio creation in e2e tests This code is very repetitive and would better live in a common function. Signed-off-by: Mike Kolesnik <mkolesni@redhat.com> --------- Signed-off-by: Mike Kolesnik <mkolesni@redhat.com> * Use `Eventually` in remote secret generation (istio-ecosystem#921) In this test the secret generation sometimes is attempted before everything is ready, leading to flaky, hard to debug, failures. Using `Eventually` to generate the secret avoids these failures. Fixes: istio-ecosystem#866 Signed-off-by: Mike Kolesnik <mkolesni@redhat.com> --------- Signed-off-by: Mike Kolesnik <mkolesni@redhat.com> Signed-off-by: Sridhar Gaddam <sgaddam@redhat.com> Signed-off-by: Francisco H <frherrer@redhat.com> Co-authored-by: Mike Kolesnik <mkolesni@redhat.com> Co-authored-by: Sridhar Gaddam <sgaddam@redhat.com> * Bump to 1.26.2, add Istio 1.26.2 (istio-ecosystem#950) Signed-off-by: Daniel Grimm <dgrimm@redhat.com> --------- Signed-off-by: Mikhail Abramov <mabramov@redhat.com> Signed-off-by: Daniel Grimm <dgrimm@redhat.com> Signed-off-by: Maxim Babushkin <mbabushk@redhat.com> Signed-off-by: Mike Kolesnik <mkolesni@redhat.com> Signed-off-by: Sridhar Gaddam <sgaddam@redhat.com> Signed-off-by: Francisco H <frherrer@redhat.com> Co-authored-by: Mikhail Abramov <mabramov@redhat.com> Co-authored-by: Istio Automation <istio-testing-bot@google.com> Co-authored-by: Daniel Grimm <dgrimm@redhat.com> Co-authored-by: Maxim Babushkin <mbabushk@redhat.com> Co-authored-by: Francisco Herrera <frherrer@redhat.com> Co-authored-by: Mike Kolesnik <mkolesni@redhat.com> Co-authored-by: Sridhar Gaddam <sgaddam@redhat.com>

* Consolidate IstioCNI creation in e2e tests This code is very repetitive and would better live in a common function. Signed-off-by: Mike Kolesnik <mkolesni@redhat.com> * Consolidate Istio creation in e2e tests This code is very repetitive and would better live in a common function. Signed-off-by: Mike Kolesnik <mkolesni@redhat.com> --------- Signed-off-by: Mike Kolesnik <mkolesni@redhat.com> Signed-off-by: Daniel Grimm <dgrimm@redhat.com>

mkolesnik requested a review from a team as a code owner June 19, 2025 08:24

istio-testing added the size/L label Jun 19, 2025

MaxBab approved these changes Jun 19, 2025

View reviewed changes

fjglira reviewed Jun 19, 2025

View reviewed changes

istio-testing added the needs-rebase label Jun 21, 2025

mkolesnik added 2 commits June 26, 2025 11:27

Consolidate IstioCNI creation in e2e tests

6deead8

This code is very repetitive and would better live in a common function. Signed-off-by: Mike Kolesnik <mkolesni@redhat.com>

Consolidate Istio creation in e2e tests

7eaf9e8

This code is very repetitive and would better live in a common function. Signed-off-by: Mike Kolesnik <mkolesni@redhat.com>

mkolesnik force-pushed the consolidate-e2e-resources branch from e644b24 to 7eaf9e8 Compare June 26, 2025 08:28

istio-testing removed the needs-rebase label Jun 26, 2025

fjglira approved these changes Jun 26, 2025

View reviewed changes

istio-testing merged commit 82efd31 into istio-ecosystem:main Jun 26, 2025
16 checks passed

openshift-service-mesh-bot pushed a commit to openshift-service-mesh-bot/sail-operator that referenced this pull request Jun 26, 2025

Automated merge

149d268

* upstream/main: Consolidate e2e resource creation (istio-ecosystem#931)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Consolidate e2e resource creation#931

Consolidate e2e resource creation#931
istio-testing merged 2 commits intoistio-ecosystem:mainfrom
mkolesnik:consolidate-e2e-resources

mkolesnik commented Jun 19, 2025

Uh oh!

codecov bot commented Jun 19, 2025 •

edited

Loading

Uh oh!

MaxBab left a comment

Uh oh!

fjglira left a comment

Uh oh!

fjglira Jun 19, 2025

Uh oh!

fjglira Jun 19, 2025

Uh oh!

mkolesnik Jun 26, 2025

Uh oh!

fjglira left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

mkolesnik commented Jun 19, 2025

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Additional information:

Uh oh!

codecov bot commented Jun 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

MaxBab left a comment

Choose a reason for hiding this comment

Uh oh!

fjglira left a comment

Choose a reason for hiding this comment

Uh oh!

fjglira Jun 19, 2025

Choose a reason for hiding this comment

Uh oh!

fjglira Jun 19, 2025

Choose a reason for hiding this comment

Uh oh!

mkolesnik Jun 26, 2025

Choose a reason for hiding this comment

Uh oh!

fjglira left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov bot commented Jun 19, 2025 •

edited

Loading