Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
2 issues found across 4 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="test/e2e/upgrade/upgrade_test.go">
<violation number="1" location="test/e2e/upgrade/upgrade_test.go:36">
P2: Check the error from namespace creation instead of ignoring it, so the test fails immediately with a clear cause.</violation>
</file>
<file name="test/e2e/upgrade/utils.go">
<violation number="1" location="test/e2e/upgrade/utils.go:201">
P2: Teardown only deletes the Deployment; the created `ClusterRoleBinding` is never deleted, leaving cluster-scoped RBAC artifacts after tests.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="test/e2e/upgrade/upgrade_test.go">
<violation number="1" location="test/e2e/upgrade/upgrade_test.go:113">
P1: Do not commit focused `FEntry` tests; they cause partial execution of the table and can fail CI due to programmatic focus.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="test/e2e/upgrade/upgrade_test.go">
<violation number="1" location="test/e2e/upgrade/upgrade_test.go:225">
P1: Remove the focused `FEntry`; it limits execution to focused specs and prevents the full upgrade matrix from running.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
vrutkovs
force-pushed
the
upgrade-testing
branch
3 times, most recently
from
ae19246 to
03996f0
Compare
Contributor
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="test/e2e/upgrade/upgrade_test.go">
<violation number="1" location="test/e2e/upgrade/upgrade_test.go:132">
P2: These new daemonset/statefulset entries still use the Deployment-only snapshot logic. If `DaemonSetMode`/`StatefulMode` switches the child workload (as the spec names imply), this test will fail to find the resource or won’t validate the correct object type. Handle DS/STS in the test logic or split these into mode-specific tables.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
vrutkovs
force-pushed
the
upgrade-testing
branch
from
1213c49 to
44b76bd
Compare
Contributor
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="test/e2e/upgrade/upgrade_test.go">
<violation number="1" location="test/e2e/upgrade/upgrade_test.go:701">
P1: VMStorage baseline is built from VMSelect spec, so the storage stability check compares against the wrong expected StatefulSet spec.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="test/e2e/upgrade/upgrade_test.go">
<violation number="1" location="test/e2e/upgrade/upgrade_test.go:236">
P2: Commenting out the VMAgent StatefulSet upgrade table removes the only active coverage for StatefulMode rollouts, so upgrade regressions in that mode can pass undetected.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
vrutkovs
force-pushed
the
upgrade-testing
branch
6 times, most recently
from
85e4880 to
4843830
Compare
Contributor
|
while fixing this issue, that was done for 0.67.0 release, order of VMAgent containers was changed, this impacts your work on this PR |
vrutkovs
force-pushed
the
upgrade-testing
branch
from
4843830 to
559b692
Compare
Collaborator
Author
|
Added a todo to sort containers by name (I don't think the order change would cause a rollout) |
vrutkovs
force-pushed
the
upgrade-testing
branch
2 times, most recently
from
35435fc to
e6b47e7
Compare
Contributor
There was a problem hiding this comment.
2 issues found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="test/e2e/upgrade/utils.go">
<violation number="1" location="test/e2e/upgrade/utils.go:303">
P2: `verifyDeployment` sanitizes only the actual PodSpec, but `snapshotDeployment` leaves expected unsanitized. This can create false diffs from container order alone.</violation>
</file>
<file name="test/e2e/upgrade/upgrade_test.go">
<violation number="1" location="test/e2e/upgrade/upgrade_test.go:90">
P2: Using `PEntry` here disables execution of these upgrade-path test cases, reducing upgrade coverage and allowing regressions for those versions to go undetected.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
vrutkovs
force-pushed
the
upgrade-testing
branch
2 times, most recently
from
13ded55 to
0811115
Compare
Contributor
There was a problem hiding this comment.
2 issues found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Makefile">
<violation number="1" location="Makefile:173">
P2: `test-e2e` now runs only upgrade tests, which drops the rest of the E2E suite from CI.</violation>
</file>
<file name="test/e2e/upgrade/upgrade_test.go">
<violation number="1" location="test/e2e/upgrade/upgrade_test.go:585">
P1: Avoid committing `FEntry`; it focuses the test suite and can skip the rest of the upgrade cases.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
vrutkovs
force-pushed
the
upgrade-testing
branch
4 times, most recently
from
42b7f68 to
4518bec
Compare
…gent upgrade tests
…, VMCluster and VLSingle settings test cases Add upgrade test cases for configurations that changed behavior since operator version 0.67.0. Adds modifier functions to verify that the operator doesn't forcibly roll out existing deployments when using: - `TargetRefs` within `UnauthorizedUserAccessSpec` (introduced in #1713) for VMAuth - `UseProxyProtocol` (introduced in #1686) for VMAuth, VMAlert, VMAlertmanager and VMCluster - `APIServerConfig`, `InsertPorts`, `IngestOnlyMode`, `StreamAggrConfig`, etc. for VMSingle (#1702) - `License` for VLSingle (#1722) - `IngestOnlyMode` for VMAgent (#1556)
…manager, VMCluster and VLSingle settings test cases
…er, VLSingle, VLCluster, VTSingle and VLAgent settings test cases
vrutkovs
force-pushed
the
upgrade-testing
branch
from
dfa333d to
b27ad9e
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Ensure that operator upgrade doesn't cause unnecessary rollouts. If there is a rollout expected (i.e. reloader change) the test case should apply changes to the initial resource to avoid a rollout.
Fixes #1952
TODO: