Repository navigation
test(sdn): drop the serviceexposure chainsaw suite left after the feature removal - #3336
Conversation
…ture removal Commit 3dbc7d3 removed ExposureClass/ServiceExposure (api/network, the controller, and the CRDs shipped in cozystack-controller/definitions) in favor of the native LoadBalancer path, but the chainsaw port that landed the next day (dd2f89d) brought the serviceexposure suite along. The suite creates network.cozystack.io/v1alpha1 fixtures against an API that no longer exists, so every full-suite run (any PR touching core paths, and every release cut) fails its two tests with "no matches for network.cozystack.io/v1alpha1". Remove the dead suite and its wiring: the suite_to_source case and its core-platform bats test in select-install (the "-" mechanism stays for the next core-platform-owned suite), and the README suite list. Co-Authored-By: Claude Fable 5 <[email protected]> Signed-off-by: Andrey Kolkov <[email protected]>
github-actions
Bot
added
area/uncategorized
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe ServiceExposure Chainsaw suite and fixtures are removed. Installer mapping no longer special-cases the suite or accepts ChangesServiceExposure suite removal
Installer mapping update
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses persistent E2E test failures caused by the presence of a test suite targeting APIs that were previously removed. By deleting the 'serviceexposure' chainsaw suite and cleaning up the associated installation logic and documentation, the PR ensures that CI/CD pipelines and E2E runs remain stable and accurate. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on Gemini (@gemini-code-assist) comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
dosubot
Bot
added
area/testing
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request removes the serviceexposure e2e chainsaw test suite and cleans up its corresponding package mapping and unit tests in hack/select-install.sh and hack/select-install_test.bats. The feedback recommends introducing a dummy core-platform mapping and an associated test case to ensure that the - mapping mechanism remains covered by tests and does not become untested dead code.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| securitygroup) echo cozystack.securitygroup-controller ; return ;; | ||
| serviceexposure) echo - ; return ;; | ||
| esac |
There was a problem hiding this comment.
Since the serviceexposure suite was removed, the - mapping mechanism in select-install.sh is now completely untested. To keep this mechanism verified and prevent it from becoming silent dead/broken code, we can introduce a dummy core-platform suite mapping for testing purposes.
| securitygroup) echo cozystack.securitygroup-controller ; return ;; | |
| serviceexposure) echo - ; return ;; | |
| esac | |
| securitygroup) echo cozystack.securitygroup-controller ; return ;; | |
| dummy-core-platform) echo - ; return ;; | |
| esac |
|
|
||
| @test "core-platform suite contributes no package and emits no warning" { | ||
| # serviceexposure is a core-platform CRD; nothing to enable, no warning. | ||
| err=$(hack/select-install.sh "serviceexposure" 2>&1 1>/dev/null) | ||
| [ -z "$err" ] | ||
| output=$(hack/select-install.sh "serviceexposure" 2>/dev/null) | ||
| [ -z "$output" ] | ||
| } | ||
|
|
||
| @test "validate passes on the real source graph and suite mapping" { |
There was a problem hiding this comment.
Add a test case using the dummy-core-platform mapping to ensure the - mechanism remains covered by unit tests.
@test "core-platform suite contributes no package and emits no warning" {
err=$(hack/select-install.sh "dummy-core-platform" 2>&1 1>/dev/null)
[ -z "$err" ]
output=$(hack/select-install.sh "dummy-core-platform" 2>/dev/null)
[ -z "$output" ]
}
@test "validate passes on the real source graph and suite mapping" {
With the serviceexposure case gone no suite returns "-", and the only executable coverage of that branch was the bats test removed alongside it. Keeping an untestable dead branch for a hypothetical future suite is not worth it — re-adding the three lines with the next core-platform suite is cheaper, and validate/closure both simplify. Co-Authored-By: Claude Fable 5 <[email protected]> Signed-off-by: Andrey Kolkov <[email protected]>
…lass The downstream-coordination checklist still directed contributors at an "externalIPs to exposureClass" migration; exposureClass was removed in 3dbc7d3 and the native replacement key is publishing.loadBalancerClass. Last leftover of the ExposureClass removal this PR sweeps. Co-Authored-By: Claude Fable 5 <[email protected]> Signed-off-by: Andrey Kolkov <[email protected]>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/agents/contributing.md`:
- Line 198: Update the migration guidance in the contributing documentation to
reference the removed exposureClass key instead of externalIPs, while keeping
publishing.loadBalancerClass as the destination and preserving the other listed
values.yaml keys unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 019f8acb-2b88-433d-bf09-768a98ce1ef9
📒 Files selected for processing (1)
docs/agents/contributing.md
| - Change the `cozy-system` namespace or the PSA pre-install hook contract → the role carries namespace-adoption logic written against the current behaviour of `packages/core/installer/templates/`. | ||
| - Add or rename a platform variant → the variant list is hardcoded in Go, in `cmd/cozystack-operator/main.go`; over there it is `cozystack_platform_variant` in `defaults/main.yml` and the README table. | ||
| - Change a `packages/core/platform/values.yaml` key the role sets — `networking.podCIDR`, `networking.podGateway`, `networking.serviceCIDR`, `networking.joinCIDR`, `publishing.host`, `publishing.apiServerEndpoint`, `publishing.externalIPs` → the role's platform-package template. The `externalIPs` to `exposureClass` migration in particular needs a coordinated PR there. | ||
| - Change a `packages/core/platform/values.yaml` key the role sets — `networking.podCIDR`, `networking.podGateway`, `networking.serviceCIDR`, `networking.joinCIDR`, `publishing.host`, `publishing.apiServerEndpoint`, `publishing.externalIPs` → the role's platform-package template. The `externalIPs` to `publishing.loadBalancerClass` migration in particular needs a coordinated PR there. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reference the removed exposureClass key, not externalIPs.
The coordinated migration is from exposureClass to publishing.loadBalancerClass; naming externalIPs makes the downstream Ansible guidance incorrect and could trigger an unrelated migration.
Proposed fix
-- Change a `packages/core/platform/values.yaml` key the role sets — `networking.podCIDR`, `networking.podGateway`, `networking.serviceCIDR`, `networking.joinCIDR`, `publishing.host`, `publishing.apiServerEndpoint`, `publishing.externalIPs` → the role's platform-package template. The `externalIPs` to `publishing.loadBalancerClass` migration in particular needs a coordinated PR there.
+- Change a `packages/core/platform/values.yaml` key the role sets — `networking.podCIDR`, `networking.podGateway`, `networking.serviceCIDR`, `networking.joinCIDR`, `publishing.host`, `publishing.apiServerEndpoint`, `publishing.externalIPs` → the role's platform-package template. The `exposureClass` to `publishing.loadBalancerClass` migration in particular needs a coordinated PR there.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - Change a `packages/core/platform/values.yaml` key the role sets — `networking.podCIDR`, `networking.podGateway`, `networking.serviceCIDR`, `networking.joinCIDR`, `publishing.host`, `publishing.apiServerEndpoint`, `publishing.externalIPs` → the role's platform-package template. The `externalIPs` to `publishing.loadBalancerClass` migration in particular needs a coordinated PR there. | |
| - Change a `packages/core/platform/values.yaml` key the role sets — `networking.podCIDR`, `networking.podGateway`, `networking.serviceCIDR`, `networking.joinCIDR`, `publishing.host`, `publishing.apiServerEndpoint`, `publishing.externalIPs` → the role's platform-package template. The `exposureClass` to `publishing.loadBalancerClass` migration in particular needs a coordinated PR there. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/agents/contributing.md` at line 198, Update the migration guidance in
the contributing documentation to reference the removed exposureClass key
instead of externalIPs, while keeping publishing.loadBalancerClass as the
destination and preserving the other listed values.yaml keys unchanged.
Scrub the last "-" mention from the --validate usage doc (the escape no longer exists, so the header asserted the opposite of the behavior), and add the negative bats case the tightened branch was missing: a suite whose hardcoded mapping names a PackageSource absent from the graph must fail validation with the "maps to ..., not a PackageSource" error. Co-Authored-By: Claude Fable 5 <[email protected]> Signed-off-by: Andrey Kolkov <[email protected]>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
hack/select-install_test.bats (1)
193-196: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
ifto assert a non-zero exit.While
err=$(...) && { ... }functions correctly without triggeringset -e(due to Bash's&&semantics), the file's header explicitly establishes a convention: "To assert a NON-zero exit, invert withifsoset -edoesn't abort the test."For consistency with other tests and the file's guidelines, consider using an
ifstatement.♻️ Proposed refactor
- err=$(hack/select-install.sh --validate "$tmp/sources" "$tmp/suites" 2>&1 1>/dev/null) && { + if err=$(hack/select-install.sh --validate "$tmp/sources" "$tmp/suites" 2>&1 1>/dev/null); then echo "expected validation to fail on a mapping to a missing source" >&2 exit 1 - } + fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@hack/select-install_test.bats` around lines 193 - 196, Update the validation-failure assertion around hack/select-install.sh to use an if statement that handles the command substitution’s non-zero exit without triggering set -e. Preserve capturing stderr in err and keep the existing failure message and exit behavior when validation unexpectedly succeeds.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@hack/select-install_test.bats`:
- Around line 193-196: Update the validation-failure assertion around
hack/select-install.sh to use an if statement that handles the command
substitution’s non-zero exit without triggering set -e. Preserve capturing
stderr in err and keep the existing failure message and exit behavior when
validation unexpectedly succeeds.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 57655de9-a90d-459b-98ae-935b677cff2e
📒 Files selected for processing (2)
hack/select-install.shhack/select-install_test.bats
🚧 Files skipped from review as they are similar to previous changes (1)
- hack/select-install.sh
Co-Authored-By: Claude Fable 5 <[email protected]> Signed-off-by: Andrey Kolkov <[email protected]>
IvanHunters
left a comment
IvanHunters
left a comment
There was a problem hiding this comment.
Reviewed hermetically. LGTM with non-blocking notes. Clean and complete removal of a stale E2E fixture; coverage is preserved, not just deleted.
Removal completeness (verified twice):
- grep for
serviceexposure/ServiceExposure/ExposureClassand for the removed API groupnetwork.cozystack.io: zero hits across the tree. - The two remaining
"-"occurrences inselect-install.sh(L16, L53) are the unrelated read-from-stdinsuites_arg="-"feature, not the removedsuite_to_sourcesentinel; correctly left intact. - No CI workflow references the suite; there is no exposure controller in
internal/controller.
Unblocking E2E: consistent with the hypothesis. The deleted fixture applied kind: ServiceExposure/ExposureClass in network.cozystack.io/v1alpha1, an API group that no longer exists, so the suite failed at apply time and passed select-install --validate only via the - sentinel, which masked the runtime break. The green E2E run is the final confirmation.
Prior concerns addressed:
- The gemini suggestion to keep the
-mapping via a dummy suite was resolved more cleanly: since serviceexposure was its sole consumer, the-sentinel was removed fromsuite_to_source()entirely and a fail-closed suite-mapping contract test was added in--validate.
Test adequacy: net coverage improves. The removed unit test only exercised the now-deleted - branch; the added validate detects a suite mapped to a source absent from the graph test pins the real fail-closed behaviour.
Non-blocking:
hack/select-install_test.bats:193: the new negative test asserts non-zero exit viaerr=$(...) && { ...; exit 1; }. It works, but the file header documents the house convention of inverting withifsoset -edoes not abort; every other test follows that idiom. Style-consistency only.
3 participants
What this PR does
Removes the dead
serviceexposurechainsaw suite and its wiring. Commit 3dbc7d3 dropped ExposureClass/ServiceExposure (api/network, the controller, and thenetwork.cozystack.ioCRDs shipped incozystack-controller/definitions) in favor of the native LoadBalancer path, but the chainsaw port that landed the next day (dd2f89d) brought the suite along, so it tests an API that no longer exists.Since then every full-suite E2E run — any PR touching core paths, and every release cut — fails
serviceexposure-deleteandserviceexposure-externalipswithno matches for network.cozystack.io/v1alpha1(see the crust-gather snapshot of e.g. run 29561399440 on #3333: the network CRDs are absent while sibling CRDs from the samedefinitions/dir, liketenantgateways.gateway.cozystack.io, are present, confirming the chart applied and the API is simply gone).Alongside the suite dir this drops the
suite_to_sourcecase and its core-platform bats test inhack/select-install*, the now-unreachable-mapping mechanism itself (no suite returns it anymore and its only executable coverage was the removed bats test; re-adding the three lines with the next core-platform-owned suite is cheaper than keeping an untestable dead branch), and the suite list entry inhack/e2e-chainsaw/README.md. Thesecuritygroupsuite from the same port is alive and green and is untouched. If the native LoadBalancer replacement needs E2E coverage, that is a new suite against the new API, not a revival of this one.Verified:
hack/select-install.sh --validatepasses (97 sources),select-install_test.bats17/17,select-e2e_test.bats13/13.Release note
Summary by CodeRabbit
Bug Fixes
serviceexposureno longer has special handling, and mappings to-are now treated as invalid unless the referenced PackageSource exists.Tests
serviceexposure, and updated the listed E2E suites accordingly.Documentation
networking.publishing.externalIPs→publishing.loadBalancerClassmigration.