Unified E2E V2 YAML setttings using envvars #3105
Conversation
📝 WalkthroughWalkthroughConsolidates and parameterizes E2E v2 test assets: removes legacy Changes
Sequence Diagram(s)sequenceDiagram
participant GH as GitHub Actions
participant Make as Make (run-v2-e2e-crud-test)
participant Go as go test (E2E v2)
participant Cfg as Config Loader
participant FS as Config File
GH->>Make: make e2e/v2 (with E2E_* env vars)
Make->>Go: invoke tests (env exported)
Go->>Cfg: Load(path=tests/v2/e2e/assets/<scenario>.yaml)
Cfg->>FS: read file (YAML/JSON)
Cfg->>Cfg: replace env placeholders (_E2E_*, _KUBECONFIG_), convert numeric strings
Cfg->>Cfg: unmarshal, Bind(), log YAML
Cfg-->>Go: bound config
Go->>Go: execute test flows per config (uses compare/toFloat64)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~35 minutes Assessment against linked issues
Out-of-scope changes
Possibly related PRs
Suggested labels
Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (31)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
[CHATOPS:HELP] ChatOps commands.
|
github-actions
bot
added
type/ci
area/makefile
actions/backport/release/v1.7
size/S
type/feature
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
Makefile (1)
372-378: Remove duplicate E2E_SEARCH_COUNT definition.E2E_SEARCH_COUNT is defined twice (lines 372 and 378) with different default values (1000 and 1000). Please remove the duplicate definition.
Apply this diff to remove the duplicate:
E2E_SEARCH_COUNT ?= 1000 E2E_UPDATE_COUNT ?= 10000 E2E_BULK_SIZE ?= 100 E2E_PORTFORWARD_ENABLED ?= true E2E_REMOVE_COUNT ?= 3 E2E_SEARCH_BY_ID_COUNT ?= 100 -E2E_SEARCH_COUNT ?= 1000 E2E_TARGET_NAME ?= vald-lb-gateway
🧹 Nitpick comments (4)
Makefile (1)
374-374: Fix indentation inconsistency.Line 374 uses tabs instead of spaces, causing inconsistent formatting. Please replace tabs with spaces to match the surrounding code style.
Apply this diff:
-E2E_BULK_SIZE ?= 100 +E2E_BULK_SIZE ?= 100tests/v2/e2e/assets/unary_crud.yaml (1)
229-233: Typo in strategy title
Parallel Search Opeation→Parallel Search Operation(same typo appears in other files – grep before fixing).tests/v2/e2e/assets/stream_crud.yaml (1)
223-224: Typo in strategy name – same as noted in unary_crud.yaml.tests/v2/e2e/assets/multi_crud.yaml (1)
224-225: Repeated “Opeation” typo – consider a global search-and-replace.Also applies to: 378-380
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
.github/e2e/index_correction.yaml(0 hunks).github/e2e/multi_crud.yaml(0 hunks).github/e2e/rollout.yaml(0 hunks).github/e2e/stream_crud.yaml(0 hunks).github/e2e/unary_crud.yaml(0 hunks).github/workflows/e2e.v2.yaml(1 hunks)Makefile(1 hunks)Makefile.d/functions.mk(1 hunks)tests/v2/e2e/assets/index_correction.yaml(6 hunks)tests/v2/e2e/assets/multi_crud.yaml(13 hunks)tests/v2/e2e/assets/readreplica.yaml(1 hunks)tests/v2/e2e/assets/rollout.yaml(5 hunks)tests/v2/e2e/assets/stream_crud.yaml(13 hunks)tests/v2/e2e/assets/unary_crud.yaml(13 hunks)
💤 Files with no reviewable changes (5)
- .github/e2e/index_correction.yaml
- .github/e2e/multi_crud.yaml
- .github/e2e/unary_crud.yaml
- .github/e2e/stream_crud.yaml
- .github/e2e/rollout.yaml
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: Matts966
PR: vdaas/vald#2928
File: pkg/gateway/lb/handler/grpc/linear_search.go:271-333
Timestamp: 2025-04-15T05:32:26.428Z
Learning: For the vald project, refactoring PRs should be kept separate from semantic changes that alter behavior. The current PR "Divide LB Gateway handler.go" is focused solely on splitting the file into smaller, more manageable components without changing functionality.
Learnt from: Matts966
PR: vdaas/vald#2928
File: pkg/gateway/lb/handler/grpc/object.go:489-576
Timestamp: 2025-04-15T05:32:18.588Z
Learning: Matts966 prefers to keep refactoring PRs (structural changes) separate from semantic/behavioral changes. The current PR "Divide LB Gateway handler.go" should focus only on refactoring.
Learnt from: Matts966
PR: vdaas/vald#2971
File: .github/e2e/unary_crud.yaml:201-205
Timestamp: 2025-05-30T13:29:28.099Z
Learning: In the Vald E2E v2 test configuration YAML files, operator values in the `expect` blocks should use lowercase format (e.g., "gt", "lt", "eq") rather than the capitalized Go constant names, as the YAML parsing handles case conversion automatically.
Learnt from: Matts966
PR: vdaas/vald#3000
File: tests/v2/e2e/crud/kubernetes_test.go:138-147
Timestamp: 2025-06-04T06:36:35.397Z
Learning: In E2E tests for the Vald project, Matts966 prefers using dual error logging (both fmt.Fprintf to stderr and t.Errorf) when handling Kubernetes operations to provide immediate debugging feedback during test execution alongside structured test failure reporting.
Learnt from: Matts966
PR: vdaas/vald#3000
File: tests/v2/e2e/assets/index_correction.yaml:119-127
Timestamp: 2025-06-04T06:38:31.386Z
Learning: In tests/v2/e2e/assets/ configuration files, Index Property check operations do not require expect blocks for local testing scenarios.
tests/v2/e2e/assets/readreplica.yaml (1)
Learnt from: Matts966
PR: #3000
File: tests/v2/e2e/assets/index_correction.yaml:119-127
Timestamp: 2025-06-04T06:38:31.386Z
Learning: In tests/v2/e2e/assets/ configuration files, Index Property check operations do not require expect blocks for local testing scenarios.
tests/v2/e2e/assets/index_correction.yaml (4)
Learnt from: Matts966
PR: #3000
File: tests/v2/e2e/assets/index_correction.yaml:119-127
Timestamp: 2025-06-04T06:38:31.386Z
Learning: In tests/v2/e2e/assets/ configuration files, Index Property check operations do not require expect blocks for local testing scenarios.
Learnt from: Matts966
PR: #2971
File: .github/e2e/unary_crud.yaml:201-205
Timestamp: 2025-05-30T13:29:28.099Z
Learning: In the Vald E2E v2 test configuration YAML files, operator values in the expect blocks should use lowercase format (e.g., "gt", "lt", "eq") rather than the capitalized Go constant names, as the YAML parsing handles case conversion automatically.
Learnt from: kpango
PR: #2564
File: charts/vald-helm-operator/crds/valdrelease.yaml:8084-8085
Timestamp: 2024-08-05T14:46:41.089Z
Learning: The kvs_background_compaction_interval property in the valdrelease.yaml file should use the pattern ^[0-9]+(ns|us|ms|s|m|h|d)$ to validate durations specified in nanoseconds, microseconds, milliseconds, seconds, minutes, hours, and days.
Learnt from: kpango
PR: #2564
File: charts/vald-helm-operator/crds/valdrelease.yaml:8084-8085
Timestamp: 2024-10-08T20:16:00.642Z
Learning: The kvs_background_compaction_interval property in the valdrelease.yaml file should use the pattern ^[0-9]+(ns|us|ms|s|m|h|d)$ to validate durations specified in nanoseconds, microseconds, milliseconds, seconds, minutes, hours, and days.
Makefile.d/functions.mk (2)
Learnt from: Matts966
PR: #3000
File: tests/v2/e2e/crud/kubernetes_test.go:138-147
Timestamp: 2025-06-04T06:36:35.397Z
Learning: In E2E tests for the Vald project, Matts966 prefers using dual error logging (both fmt.Fprintf to stderr and t.Errorf) when handling Kubernetes operations to provide immediate debugging feedback during test execution alongside structured test failure reporting.
Learnt from: Matts966
PR: #2971
File: .github/e2e/unary_crud.yaml:201-205
Timestamp: 2025-05-30T13:29:28.099Z
Learning: In the Vald E2E v2 test configuration YAML files, operator values in the expect blocks should use lowercase format (e.g., "gt", "lt", "eq") rather than the capitalized Go constant names, as the YAML parsing handles case conversion automatically.
.github/workflows/e2e.v2.yaml (3)
Learnt from: Matts966
PR: #3000
File: tests/v2/e2e/assets/index_correction.yaml:119-127
Timestamp: 2025-06-04T06:38:31.386Z
Learning: In tests/v2/e2e/assets/ configuration files, Index Property check operations do not require expect blocks for local testing scenarios.
Learnt from: Matts966
PR: #2971
File: .github/e2e/unary_crud.yaml:201-205
Timestamp: 2025-05-30T13:29:28.099Z
Learning: In the Vald E2E v2 test configuration YAML files, operator values in the expect blocks should use lowercase format (e.g., "gt", "lt", "eq") rather than the capitalized Go constant names, as the YAML parsing handles case conversion automatically.
Learnt from: Matts966
PR: #3000
File: tests/v2/e2e/crud/kubernetes_test.go:138-147
Timestamp: 2025-06-04T06:36:35.397Z
Learning: In E2E tests for the Vald project, Matts966 prefers using dual error logging (both fmt.Fprintf to stderr and t.Errorf) when handling Kubernetes operations to provide immediate debugging feedback during test execution alongside structured test failure reporting.
tests/v2/e2e/assets/rollout.yaml (2)
Learnt from: Matts966
PR: #3000
File: tests/v2/e2e/assets/index_correction.yaml:119-127
Timestamp: 2025-06-04T06:38:31.386Z
Learning: In tests/v2/e2e/assets/ configuration files, Index Property check operations do not require expect blocks for local testing scenarios.
Learnt from: Matts966
PR: #2971
File: .github/e2e/unary_crud.yaml:201-205
Timestamp: 2025-05-30T13:29:28.099Z
Learning: In the Vald E2E v2 test configuration YAML files, operator values in the expect blocks should use lowercase format (e.g., "gt", "lt", "eq") rather than the capitalized Go constant names, as the YAML parsing handles case conversion automatically.
tests/v2/e2e/assets/unary_crud.yaml (3)
Learnt from: Matts966
PR: #3000
File: tests/v2/e2e/assets/index_correction.yaml:119-127
Timestamp: 2025-06-04T06:38:31.386Z
Learning: In tests/v2/e2e/assets/ configuration files, Index Property check operations do not require expect blocks for local testing scenarios.
Learnt from: Matts966
PR: #2971
File: .github/e2e/unary_crud.yaml:201-205
Timestamp: 2025-05-30T13:29:28.099Z
Learning: In the Vald E2E v2 test configuration YAML files, operator values in the expect blocks should use lowercase format (e.g., "gt", "lt", "eq") rather than the capitalized Go constant names, as the YAML parsing handles case conversion automatically.
Learnt from: Matts966
PR: #2996
File: tests/v2/e2e/crud/grpc_test.go:82-165
Timestamp: 2025-06-03T04:21:14.579Z
Learning: In the handleGRPCWithStatusCode function in tests/v2/e2e/crud/grpc_test.go, the early return at line 159 inside the expectations loop is intentional behavior. The function implements OR logic where any one expectation passing is sufficient for success, rather than requiring all expectations to pass.
tests/v2/e2e/assets/stream_crud.yaml (3)
Learnt from: Matts966
PR: #3000
File: tests/v2/e2e/assets/index_correction.yaml:119-127
Timestamp: 2025-06-04T06:38:31.386Z
Learning: In tests/v2/e2e/assets/ configuration files, Index Property check operations do not require expect blocks for local testing scenarios.
Learnt from: Matts966
PR: #2971
File: .github/e2e/unary_crud.yaml:201-205
Timestamp: 2025-05-30T13:29:28.099Z
Learning: In the Vald E2E v2 test configuration YAML files, operator values in the expect blocks should use lowercase format (e.g., "gt", "lt", "eq") rather than the capitalized Go constant names, as the YAML parsing handles case conversion automatically.
Learnt from: Matts966
PR: #2996
File: tests/v2/e2e/crud/grpc_test.go:82-165
Timestamp: 2025-06-03T04:21:14.579Z
Learning: In the handleGRPCWithStatusCode function in tests/v2/e2e/crud/grpc_test.go, the early return at line 159 inside the expectations loop is intentional behavior. The function implements OR logic where any one expectation passing is sufficient for success, rather than requiring all expectations to pass.
tests/v2/e2e/assets/multi_crud.yaml (3)
Learnt from: Matts966
PR: #3000
File: tests/v2/e2e/assets/index_correction.yaml:119-127
Timestamp: 2025-06-04T06:38:31.386Z
Learning: In tests/v2/e2e/assets/ configuration files, Index Property check operations do not require expect blocks for local testing scenarios.
Learnt from: Matts966
PR: #2971
File: .github/e2e/unary_crud.yaml:201-205
Timestamp: 2025-05-30T13:29:28.099Z
Learning: In the Vald E2E v2 test configuration YAML files, operator values in the expect blocks should use lowercase format (e.g., "gt", "lt", "eq") rather than the capitalized Go constant names, as the YAML parsing handles case conversion automatically.
Learnt from: Matts966
PR: #2996
File: tests/v2/e2e/crud/grpc_test.go:82-165
Timestamp: 2025-06-03T04:21:14.579Z
Learning: In the handleGRPCWithStatusCode function in tests/v2/e2e/crud/grpc_test.go, the early return at line 159 inside the expectations loop is intentional behavior. The function implements OR logic where any one expectation passing is sufficient for success, rather than requiring all expectations to pass.
🪛 YAMLlint (1.37.1)
.github/workflows/e2e.v2.yaml
[error] 210-210: syntax error: found character '\t' that cannot start any token
(syntax)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: check-format-diff
- GitHub Check: check-format-diff
- GitHub Check: check-format-diff
- GitHub Check: check-format-diff
- GitHub Check: check-format-diff
- GitHub Check: check-format-diff
- GitHub Check: check-format-diff
- GitHub Check: check-format-diff
- GitHub Check: check-format-diff
- GitHub Check: check-format-diff
🔇 Additional comments (10)
tests/v2/e2e/assets/readreplica.yaml (1)
24-24: LGTM! Environment variable parameterization implemented correctly.The addition of
kube_config: _KUBECONFIG_placeholder aligns perfectly with the PR objective of unifying E2E V2 YAML settings using environment variables. This makes the configuration more flexible and environment-agnostic..github/workflows/e2e.v2.yaml (1)
209-215: LGTM! Path migration and parameterization implemented correctly.The configuration file path change from
.github/e2e/totests/v2/e2e/assets/and the explicit environment variable passing align perfectly with the PR objective of unifying E2E V2 YAML settings using environment variables.Makefile.d/functions.mk (1)
146-151: LGTM! Environment variable exports added correctly.The addition of six new environment variable exports (E2E_PARALLELISM, E2E_INSERT_COUNT, E2E_QPS, E2E_SEARCH_COUNT, E2E_UPDATE_COUNT, E2E_BULK_SIZE) in the
run-v2-e2e-crud-testfunction enables parameterized test execution. This change aligns perfectly with the PR objective of unifying E2E V2 YAML settings using environment variables.Makefile (1)
369-374: LGTM! Enhanced E2E parameterization implemented.The addition of new E2E configuration variables (E2E_PARALLELISM, E2E_QPS, E2E_SEARCH_COUNT, E2E_UPDATE_COUNT, E2E_BULK_SIZE) and the increase in E2E_INSERT_COUNT provide finer control over test parameters, aligning with the PR objective of unifying E2E V2 YAML settings using environment variables.
tests/v2/e2e/assets/index_correction.yaml (5)
24-24: LGTM! Kubernetes configuration parameterized correctly.The replacement of hardcoded kube_config path with
_KUBECONFIG_placeholder aligns with the PR objective of unifying E2E V2 YAML settings using environment variables.
128-132: LGTM! Enhanced IndexProperty validation added.The new expect block for IndexProperty check improves test robustness by validating that the details array contains at least 3 elements. The lowercase operator format
geis correct per Vald E2E conventions.
150-152: LGTM! Test parameters parameterized correctly.The replacement of hardcoded values with environment variable placeholders (
_E2E_PARALLELISM_,_E2E_INSERT_COUNT_,_E2E_QPS_) enables flexible test configuration, aligning perfectly with the parameterization objectives.
157-157: LGTM! Retry timeout enhances test robustness.The addition of
retry_until_success_timeout: 5mimproves test reliability by allowing IndexInfo operations to retry for up to 5 minutes before failing.
161-161: LGTM! Expected values parameterized consistently.The replacement of hardcoded expected values with
_E2E_INSERT_COUNT_placeholder ensures that expectations scale with the actual insert count, making tests more maintainable and flexible.Also applies to: 190-190, 211-211, 235-235
tests/v2/e2e/assets/rollout.yaml (1)
24-25: Parameterization looks solid
kube_configand every numeric field are now driven by env-var placeholders. This keeps the asset fully CI-friendly and consistent with the new Makefile exports.
Deploying vald with
|
| Latest commit: |
70d892c
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://d8fc7393.vald.pages.dev |
| Branch Preview URL: | https://feature-unify-e2e-yaml.vald.pages.dev |
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
Makefile (1)
378-384: Consolidate duplicate E2E variable definitionsThe Makefile currently defines the same E2E_* variables twice—one of them with conflicting defaults—so please remove or merge the duplicates and ensure you’re using the intended value:
• Makefile
- Lines 372 & 378:
• E2E_SEARCH_COUNT = 1000 (duplicate)
→ Keep a single definition.- Lines 373 & 384:
• E2E_UPDATE_COUNT = 10000
• E2E_UPDATE_COUNT = 10
→ Decide which default is correct and remove the other.
🧹 Nitpick comments (2)
tests/v2/e2e/assets/unary_crud.yaml (1)
229-229: Minor spelling nit – “Opeation”
Parallel Search Opeation→Parallel Search Operation(appears in several files).tests/v2/e2e/assets/stream_crud.yaml (1)
430-444: Consistency: bulk sizes hard-coded to 10You parameterised almost every numeric, but the
bulk_sizeforUpdate/Upsertis still literal10.
If this is deliberate (e.g., protocol constraint) ignore; otherwise replace with_E2E_BULK_SIZE_for symmetry.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
.github/e2e/index_correction.yaml(0 hunks).github/e2e/multi_crud.yaml(0 hunks).github/e2e/rollout.yaml(0 hunks).github/e2e/stream_crud.yaml(0 hunks).github/e2e/unary_crud.yaml(0 hunks).github/workflows/e2e.v2.yaml(1 hunks)Makefile(1 hunks)Makefile.d/functions.mk(1 hunks)tests/v2/e2e/assets/index_correction.yaml(6 hunks)tests/v2/e2e/assets/multi_crud.yaml(13 hunks)tests/v2/e2e/assets/readreplica.yaml(1 hunks)tests/v2/e2e/assets/rollout.yaml(5 hunks)tests/v2/e2e/assets/stream_crud.yaml(13 hunks)tests/v2/e2e/assets/unary_crud.yaml(13 hunks)
💤 Files with no reviewable changes (5)
- .github/e2e/rollout.yaml
- .github/e2e/index_correction.yaml
- .github/e2e/multi_crud.yaml
- .github/e2e/unary_crud.yaml
- .github/e2e/stream_crud.yaml
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: Matts966
PR: vdaas/vald#2928
File: pkg/gateway/lb/handler/grpc/linear_search.go:271-333
Timestamp: 2025-04-15T05:32:26.428Z
Learning: For the vald project, refactoring PRs should be kept separate from semantic changes that alter behavior. The current PR "Divide LB Gateway handler.go" is focused solely on splitting the file into smaller, more manageable components without changing functionality.
Learnt from: Matts966
PR: vdaas/vald#2928
File: pkg/gateway/lb/handler/grpc/object.go:489-576
Timestamp: 2025-04-15T05:32:18.588Z
Learning: Matts966 prefers to keep refactoring PRs (structural changes) separate from semantic/behavioral changes. The current PR "Divide LB Gateway handler.go" should focus only on refactoring.
Learnt from: Matts966
PR: vdaas/vald#2971
File: .github/e2e/unary_crud.yaml:201-205
Timestamp: 2025-05-30T13:29:28.099Z
Learning: In the Vald E2E v2 test configuration YAML files, operator values in the `expect` blocks should use lowercase format (e.g., "gt", "lt", "eq") rather than the capitalized Go constant names, as the YAML parsing handles case conversion automatically.
Learnt from: Matts966
PR: vdaas/vald#3000
File: tests/v2/e2e/assets/index_correction.yaml:119-127
Timestamp: 2025-06-04T06:38:31.386Z
Learning: In tests/v2/e2e/assets/ configuration files, Index Property check operations do not require expect blocks for local testing scenarios.
tests/v2/e2e/assets/readreplica.yaml (1)
Learnt from: Matts966
PR: #3000
File: tests/v2/e2e/assets/index_correction.yaml:119-127
Timestamp: 2025-06-04T06:38:31.386Z
Learning: In tests/v2/e2e/assets/ configuration files, Index Property check operations do not require expect blocks for local testing scenarios.
.github/workflows/e2e.v2.yaml (2)
Learnt from: Matts966
PR: #3000
File: tests/v2/e2e/assets/index_correction.yaml:119-127
Timestamp: 2025-06-04T06:38:31.386Z
Learning: In tests/v2/e2e/assets/ configuration files, Index Property check operations do not require expect blocks for local testing scenarios.
Learnt from: Matts966
PR: #3000
File: tests/v2/e2e/crud/kubernetes_test.go:138-147
Timestamp: 2025-06-04T06:36:35.397Z
Learning: In E2E tests for the Vald project, Matts966 prefers using dual error logging (both fmt.Fprintf to stderr and t.Errorf) when handling Kubernetes operations to provide immediate debugging feedback during test execution alongside structured test failure reporting.
Makefile.d/functions.mk (2)
Learnt from: Matts966
PR: #3000
File: tests/v2/e2e/crud/kubernetes_test.go:138-147
Timestamp: 2025-06-04T06:36:35.397Z
Learning: In E2E tests for the Vald project, Matts966 prefers using dual error logging (both fmt.Fprintf to stderr and t.Errorf) when handling Kubernetes operations to provide immediate debugging feedback during test execution alongside structured test failure reporting.
Learnt from: Matts966
PR: #2971
File: .github/e2e/unary_crud.yaml:201-205
Timestamp: 2025-05-30T13:29:28.099Z
Learning: In the Vald E2E v2 test configuration YAML files, operator values in the expect blocks should use lowercase format (e.g., "gt", "lt", "eq") rather than the capitalized Go constant names, as the YAML parsing handles case conversion automatically.
tests/v2/e2e/assets/rollout.yaml (2)
Learnt from: Matts966
PR: #3000
File: tests/v2/e2e/assets/index_correction.yaml:119-127
Timestamp: 2025-06-04T06:38:31.386Z
Learning: In tests/v2/e2e/assets/ configuration files, Index Property check operations do not require expect blocks for local testing scenarios.
Learnt from: Matts966
PR: #2971
File: .github/e2e/unary_crud.yaml:201-205
Timestamp: 2025-05-30T13:29:28.099Z
Learning: In the Vald E2E v2 test configuration YAML files, operator values in the expect blocks should use lowercase format (e.g., "gt", "lt", "eq") rather than the capitalized Go constant names, as the YAML parsing handles case conversion automatically.
tests/v2/e2e/assets/unary_crud.yaml (3)
Learnt from: Matts966
PR: #3000
File: tests/v2/e2e/assets/index_correction.yaml:119-127
Timestamp: 2025-06-04T06:38:31.386Z
Learning: In tests/v2/e2e/assets/ configuration files, Index Property check operations do not require expect blocks for local testing scenarios.
Learnt from: Matts966
PR: #2971
File: .github/e2e/unary_crud.yaml:201-205
Timestamp: 2025-05-30T13:29:28.099Z
Learning: In the Vald E2E v2 test configuration YAML files, operator values in the expect blocks should use lowercase format (e.g., "gt", "lt", "eq") rather than the capitalized Go constant names, as the YAML parsing handles case conversion automatically.
Learnt from: Matts966
PR: #2996
File: tests/v2/e2e/crud/grpc_test.go:82-165
Timestamp: 2025-06-03T04:21:14.579Z
Learning: In the handleGRPCWithStatusCode function in tests/v2/e2e/crud/grpc_test.go, the early return at line 159 inside the expectations loop is intentional behavior. The function implements OR logic where any one expectation passing is sufficient for success, rather than requiring all expectations to pass.
tests/v2/e2e/assets/stream_crud.yaml (3)
Learnt from: Matts966
PR: #3000
File: tests/v2/e2e/assets/index_correction.yaml:119-127
Timestamp: 2025-06-04T06:38:31.386Z
Learning: In tests/v2/e2e/assets/ configuration files, Index Property check operations do not require expect blocks for local testing scenarios.
Learnt from: Matts966
PR: #2971
File: .github/e2e/unary_crud.yaml:201-205
Timestamp: 2025-05-30T13:29:28.099Z
Learning: In the Vald E2E v2 test configuration YAML files, operator values in the expect blocks should use lowercase format (e.g., "gt", "lt", "eq") rather than the capitalized Go constant names, as the YAML parsing handles case conversion automatically.
Learnt from: Matts966
PR: #2996
File: tests/v2/e2e/crud/grpc_test.go:82-165
Timestamp: 2025-06-03T04:21:14.579Z
Learning: In the handleGRPCWithStatusCode function in tests/v2/e2e/crud/grpc_test.go, the early return at line 159 inside the expectations loop is intentional behavior. The function implements OR logic where any one expectation passing is sufficient for success, rather than requiring all expectations to pass.
tests/v2/e2e/assets/index_correction.yaml (4)
Learnt from: Matts966
PR: #3000
File: tests/v2/e2e/assets/index_correction.yaml:119-127
Timestamp: 2025-06-04T06:38:31.386Z
Learning: In tests/v2/e2e/assets/ configuration files, Index Property check operations do not require expect blocks for local testing scenarios.
Learnt from: Matts966
PR: #2971
File: .github/e2e/unary_crud.yaml:201-205
Timestamp: 2025-05-30T13:29:28.099Z
Learning: In the Vald E2E v2 test configuration YAML files, operator values in the expect blocks should use lowercase format (e.g., "gt", "lt", "eq") rather than the capitalized Go constant names, as the YAML parsing handles case conversion automatically.
Learnt from: kpango
PR: #2564
File: charts/vald-helm-operator/crds/valdrelease.yaml:8084-8085
Timestamp: 2024-08-05T14:46:41.089Z
Learning: The kvs_background_compaction_interval property in the valdrelease.yaml file should use the pattern ^[0-9]+(ns|us|ms|s|m|h|d)$ to validate durations specified in nanoseconds, microseconds, milliseconds, seconds, minutes, hours, and days.
Learnt from: kpango
PR: #2564
File: charts/vald-helm-operator/crds/valdrelease.yaml:8084-8085
Timestamp: 2024-10-08T20:16:00.642Z
Learning: The kvs_background_compaction_interval property in the valdrelease.yaml file should use the pattern ^[0-9]+(ns|us|ms|s|m|h|d)$ to validate durations specified in nanoseconds, microseconds, milliseconds, seconds, minutes, hours, and days.
tests/v2/e2e/assets/multi_crud.yaml (3)
Learnt from: Matts966
PR: #3000
File: tests/v2/e2e/assets/index_correction.yaml:119-127
Timestamp: 2025-06-04T06:38:31.386Z
Learning: In tests/v2/e2e/assets/ configuration files, Index Property check operations do not require expect blocks for local testing scenarios.
Learnt from: Matts966
PR: #2971
File: .github/e2e/unary_crud.yaml:201-205
Timestamp: 2025-05-30T13:29:28.099Z
Learning: In the Vald E2E v2 test configuration YAML files, operator values in the expect blocks should use lowercase format (e.g., "gt", "lt", "eq") rather than the capitalized Go constant names, as the YAML parsing handles case conversion automatically.
Learnt from: Matts966
PR: #2996
File: tests/v2/e2e/crud/grpc_test.go:82-165
Timestamp: 2025-06-03T04:21:14.579Z
Learning: In the handleGRPCWithStatusCode function in tests/v2/e2e/crud/grpc_test.go, the early return at line 159 inside the expectations loop is intentional behavior. The function implements OR logic where any one expectation passing is sufficient for success, rather than requiring all expectations to pass.
🪛 YAMLlint (1.37.1)
.github/workflows/e2e.v2.yaml
[error] 210-210: syntax error: found character '\t' that cannot start any token
(syntax)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: check-format-diff
- GitHub Check: check-format-diff
- GitHub Check: check-format-diff
- GitHub Check: check-format-diff
- GitHub Check: check-format-diff
- GitHub Check: check-format-diff
- GitHub Check: check-format-diff
- GitHub Check: check-format-diff
- GitHub Check: check-format-diff
- GitHub Check: check-format-diff
🔇 Additional comments (14)
tests/v2/e2e/assets/readreplica.yaml (1)
24-24: LGTM! Consistent parameterization of Kubernetes config.The change from a hardcoded path to the
_KUBECONFIG_placeholder aligns perfectly with the unified E2E YAML settings approach and enables flexible configuration across different environments..github/workflows/e2e.v2.yaml (1)
209-215: LGTM! Workflow correctly updated for parameterized E2E testing.The changes properly:
- Update the E2E config path to use the new
tests/v2/e2e/assets/directory- Pass the new environment variables to control test parameters
- Maintain the same test coverage with improved configurability
Makefile.d/functions.mk (1)
146-151: LGTM! Environment variables correctly exported for E2E test execution.The addition of these six environment variables ensures the parameterized YAML configurations receive the proper values during test execution. The variable names match the placeholders used in the asset files.
Makefile (1)
369-374: LGTM! Good default values for parameterized E2E testing.The new environment variables provide sensible defaults and the increase in
E2E_INSERT_COUNTfrom 10,000 to 60,000 suggests more comprehensive testing coverage.tests/v2/e2e/assets/index_correction.yaml (5)
24-24: LGTM! Kubernetes config path properly parameterized.Consistent with the unified approach across all E2E asset files.
128-132: Good addition of validation for Index Property check.The new expect block adds robustness by verifying that the details array has at least 3 elements with a successful status code.
150-152: LGTM! Insert operation properly parameterized.The replacement of hardcoded values with environment variable placeholders enables flexible test configuration while maintaining the same test logic.
157-157: Good improvement: retry timeout added for reliability.The
retry_until_success_timeout: 5maddition makes the IndexInfo operation more resilient to timing variations in test environments.
161-161: LGTM! Consistent parameterization of expected values.All hardcoded count expectations have been properly replaced with the
_E2E_INSERT_COUNT_placeholder, maintaining test logic while enabling dynamic configuration.Also applies to: 190-190, 211-211, 235-235
tests/v2/e2e/assets/rollout.yaml (1)
24-24: Verify_KUBECONFIG_is exported during CI
kube_confignow points to the_KUBECONFIG_placeholder. Double-check that the Makefile / workflow actually exportsKUBECONFIG(or sets this placeholder) before the test runs, otherwise port-forwarding will fail at start-up.tests/v2/e2e/assets/unary_crud.yaml (2)
192-196: Expectation block looks good – lower-casegecomplies with YAML parser rules and the added size assertion strengthens the test.
228-228: Operatorgtvs exact equalityYou switched the assertion earlier from
gtto equality in other files (rollout, stream, multi).
Here it’s stillgt. Confirm that this is intentionally looser; if you really want exact match, change to:- op: gt - value: _E2E_INSERT_COUNT_ + op: eq + value: _E2E_INSERT_COUNT_tests/v2/e2e/assets/multi_crud.yaml (2)
189-193: Good addition of size assertionThe new
gecheck makes the IndexProperty pre-flight stronger. 👍
231-261: Bulk-size placeholder added – confirm Makefile exportsE2E_BULK_SIZEAll multi-search executions now rely on
_E2E_BULK_SIZE_. Make sure it’s set; CI failures here are noisy and hard to trace.
|
[WARNING:INTCFG] Changes in |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/config/config.go (1)
99-122: Effective implementation for deep environment variable substitution.The refactored approach successfully enables recursive environment variable replacement throughout nested configuration structures. The intermediate JSON marshaling provides a clean way to transform the generic map back to the target struct type.
For future optimization, consider a direct reflection-based approach to avoid the JSON round-trip overhead, though the current implementation is perfectly suitable for typical configuration file sizes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/config/config.go(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: Matts966
PR: vdaas/vald#2928
File: pkg/gateway/lb/handler/grpc/linear_search.go:271-333
Timestamp: 2025-04-15T05:32:26.428Z
Learning: For the vald project, refactoring PRs should be kept separate from semantic changes that alter behavior. The current PR "Divide LB Gateway handler.go" is focused solely on splitting the file into smaller, more manageable components without changing functionality.
Learnt from: Matts966
PR: vdaas/vald#2928
File: pkg/gateway/lb/handler/grpc/object.go:489-576
Timestamp: 2025-04-15T05:32:18.588Z
Learning: Matts966 prefers to keep refactoring PRs (structural changes) separate from semantic/behavioral changes. The current PR "Divide LB Gateway handler.go" should focus only on refactoring.
Learnt from: Matts966
PR: vdaas/vald#2971
File: .github/e2e/unary_crud.yaml:201-205
Timestamp: 2025-05-30T13:29:28.099Z
Learning: In the Vald E2E v2 test configuration YAML files, operator values in the `expect` blocks should use lowercase format (e.g., "gt", "lt", "eq") rather than the capitalized Go constant names, as the YAML parsing handles case conversion automatically.
Learnt from: Matts966
PR: vdaas/vald#3000
File: tests/v2/e2e/crud/kubernetes_test.go:138-147
Timestamp: 2025-06-04T06:36:35.397Z
Learning: In E2E tests for the Vald project, Matts966 prefers using dual error logging (both fmt.Fprintf to stderr and t.Errorf) when handling Kubernetes operations to provide immediate debugging feedback during test execution alongside structured test failure reporting.
Learnt from: Matts966
PR: vdaas/vald#3000
File: tests/v2/e2e/assets/index_correction.yaml:119-127
Timestamp: 2025-06-04T06:38:31.386Z
Learning: In tests/v2/e2e/assets/ configuration files, Index Property check operations do not require expect blocks for local testing scenarios.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (17)
- GitHub Check: e2e (stream_crud, helm-chart, k3d, null)
- GitHub Check: build / dump-contexts-to-log
- GitHub Check: e2e (stream_crud, helm-chart, k3d, null)
- GitHub Check: build / dump-contexts-to-log
- GitHub Check: e2e (stream_crud, helm-chart, k3d, null)
- GitHub Check: build / dump-contexts-to-log
- GitHub Check: e2e (stream_crud, helm-chart, k3d, null)
- GitHub Check: e2e (stream_crud, helm-chart, k3d, null)
- GitHub Check: e2e (stream_crud, helm-chart, k3d, null)
- GitHub Check: build / dump-contexts-to-log
- GitHub Check: e2e (stream_crud, helm-chart, k3d, null)
- GitHub Check: build / dump-contexts-to-log
- GitHub Check: e2e (stream_crud, helm-chart, k3d, null)
- GitHub Check: build / dump-contexts-to-log
- GitHub Check: e2e (stream_crud, helm-chart, k3d, null)
- GitHub Check: build / dump-contexts-to-log
- GitHub Check: e2e (stream_crud, helm-chart, k3d, null)
🔇 Additional comments (1)
internal/config/config.go (1)
65-82: LGTM! Well-implemented recursive environment variable substitution.The function correctly handles nested data structures (strings, slices, maps) and applies environment variable resolution recursively. The in-place mutation is acceptable for this internal use case within the
Readfunction.
Matts966
force-pushed
the
feature/unify-e2e-yaml
branch
from
585fb40 to
685855d
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3105 +/- ##
=======================================
Coverage 17.34% 17.34%
=======================================
Files 129 129
Lines 14544 14544
=======================================
Hits 2522 2522
Misses 11731 11731
Partials 291 291 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Matts966
force-pushed
the
feature/unify-e2e-yaml
branch
from
685855d to
f315633
Compare
|
/format |
|
/format |
|
[FORMAT] Updating license headers and formatting go codes triggered by Matts966. |
Signed-off-by: Vdaas CI <vald@vdaas.org>
Matts966
force-pushed
the
feature/unify-e2e-yaml
branch
from
581845c to
cb4b8a3
Compare
|
[FORMAT] Failed to format. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
tests/v2/e2e/assets/index_correction.yaml (1)
150-152: Insert parameterization via envvars — OK; reuse verification from multi_crudPlaceholders are consistent with Makefile/workflow. See my earlier verification script to ensure all placeholders are exported and expanded at runtime.
Also applies to: 157-161
tests/v2/e2e/assets/unary_crud.yaml (2)
215-217: Insert parameterization — OK; verify substitution wiringPlaceholders E2E_PARALLELISM, E2E_INSERT_COUNT, E2E_QPS, and E2E_EXPECTED_INDEX are consistent. Please rely on the earlier verification script to confirm exports and expansion.
Also applies to: 222-222, 226-226
398-400: Guardrail: ensure UPDATE/UPSERT counts don’t exceed inserted countOffsets are reset to 0; if E2E_UPDATE_COUNT > E2E_INSERT_COUNT, updates/upserts will target non-existent IDs and may flake. Please run the earlier guardrail script to confirm defaults and CI values keep the invariant E2E_UPDATE_COUNT <= E2E_INSERT_COUNT.
Also applies to: 443-447
🧹 Nitpick comments (2)
tests/v2/e2e/assets/multi_crud.yaml (1)
224-224: Fix typo: “Opeation” → “Operation”Purely cosmetic but aids readability in reports and logs.
- name: Parallel Search Opeation (Search, SearchByID, LinearSearch, LinearSearchByID) x (ConcurrentQueue, SortSlice, SortPoolSlice, PairingHeap) = 16 + name: Parallel Search Operation (Search, SearchByID, LinearSearch, LinearSearchByID) x (ConcurrentQueue, SortSlice, SortPoolSlice, PairingHeap) = 16 ... - name: GetObject/Exists/GetTimestamp Opeation + name: GetObject/Exists/GetTimestamp OperationAlso applies to: 379-379
tests/v2/e2e/assets/unary_crud.yaml (1)
228-228: Fix typo: “Opeation” → “Operation”For readability in logs/reports.
- name: Parallel Search Opeation (Search, SearchByID, LinearSearch, LinearSearchByID) x (ConcurrentQueue, SortSlice, SortPoolSlice, PairingHeap) = 16 + name: Parallel Search Operation (Search, SearchByID, LinearSearch, LinearSearchByID) x (ConcurrentQueue, SortSlice, SortPoolSlice, PairingHeap) = 16 ... - name: GetObject/Exists/GetTimestamp Opeation + name: GetObject/Exists/GetTimestamp OperationAlso applies to: 367-367
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
.gitfiles(1 hunks).github/e2e/index_correction.yaml(0 hunks).github/e2e/multi_crud.yaml(0 hunks).github/e2e/rollout.yaml(0 hunks).github/e2e/stream_crud.yaml(0 hunks).github/e2e/unary_crud.yaml(0 hunks).github/workflows/e2e.v2.yaml(1 hunks)Makefile(1 hunks)Makefile.d/functions.mk(1 hunks)tests/v2/e2e/assets/index_correction.yaml(7 hunks)tests/v2/e2e/assets/multi_crud.yaml(13 hunks)tests/v2/e2e/assets/readreplica.yaml(1 hunks)tests/v2/e2e/assets/rollout.yaml(5 hunks)tests/v2/e2e/assets/stream_crud.yaml(13 hunks)tests/v2/e2e/assets/unary_crud.yaml(13 hunks)tests/v2/e2e/config/config.go(3 hunks)tests/v2/e2e/crud/grpc_test.go(1 hunks)
💤 Files with no reviewable changes (5)
- .github/e2e/rollout.yaml
- .github/e2e/unary_crud.yaml
- .github/e2e/index_correction.yaml
- .github/e2e/multi_crud.yaml
- .github/e2e/stream_crud.yaml
🚧 Files skipped from review as they are similar to previous changes (9)
- .github/workflows/e2e.v2.yaml
- tests/v2/e2e/crud/grpc_test.go
- .gitfiles
- Makefile
- Makefile.d/functions.mk
- tests/v2/e2e/assets/readreplica.yaml
- tests/v2/e2e/assets/rollout.yaml
- tests/v2/e2e/config/config.go
- tests/v2/e2e/assets/stream_crud.yaml
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-06-04T06:38:31.386Z
Learnt from: Matts966
PR: vdaas/vald#3000
File: tests/v2/e2e/assets/index_correction.yaml:119-127
Timestamp: 2025-06-04T06:38:31.386Z
Learning: In tests/v2/e2e/assets/ configuration files, Index Property check operations do not require expect blocks for local testing scenarios.
Applied to files:
tests/v2/e2e/assets/unary_crud.yamltests/v2/e2e/assets/multi_crud.yamltests/v2/e2e/assets/index_correction.yaml
📚 Learning: 2025-05-30T13:29:28.099Z
Learnt from: Matts966
PR: vdaas/vald#2971
File: .github/e2e/unary_crud.yaml:201-205
Timestamp: 2025-05-30T13:29:28.099Z
Learning: In the Vald E2E v2 test configuration YAML files, operator values in the `expect` blocks should use lowercase format (e.g., "gt", "lt", "eq") rather than the capitalized Go constant names, as the YAML parsing handles case conversion automatically.
Applied to files:
tests/v2/e2e/assets/multi_crud.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (27)
- GitHub Check: dump-contexts-to-log
- GitHub Check: CodeQL Analyze (actions)
- GitHub Check: CodeQL Analyze (rust)
- GitHub Check: dump-contexts-to-log
- GitHub Check: dump-contexts-to-log
- GitHub Check: CodeQL Analyze (actions)
- GitHub Check: CodeQL Analyze (rust)
- GitHub Check: dump-contexts-to-log
- GitHub Check: dump-contexts-to-log
- GitHub Check: CodeQL Analyze (actions)
- GitHub Check: CodeQL Analyze (rust)
- GitHub Check: dump-contexts-to-log
- GitHub Check: CodeQL Analyze (actions)
- GitHub Check: CodeQL Analyze (rust)
- GitHub Check: dump-contexts-to-log
- GitHub Check: CodeQL Analyze (actions)
- GitHub Check: CodeQL Analyze (rust)
- GitHub Check: CodeQL Analyze (rust)
- GitHub Check: runner / golangci-lint
- GitHub Check: CodeQL Analyze (rust)
- GitHub Check: runner / golangci-lint
- GitHub Check: CodeQL Analyze (rust)
- GitHub Check: runner / golangci-lint
- GitHub Check: CodeQL Analyze (rust)
- GitHub Check: runner / golangci-lint
- GitHub Check: CodeQL Analyze (rust)
- GitHub Check: runner / golangci-lint
🔇 Additional comments (15)
tests/v2/e2e/assets/multi_crud.yaml (7)
24-24: Env-driven kubeconfig looks goodUsing KUBECONFIG aligns with the new envvar-based approach and avoids host-specific paths.
189-193: IndexProperty expect added — OKThe lowercased operator ge is correct. Note: per our past learnings, Index Property checks don’t strictly need expect blocks for local runs; keeping it here is fine and improves CI signal.
231-234: Search/Get/Exists/Timestamp steps consistently parameterized — LGTMConsistent usage of E2E_BULK_SIZE, E2E_PARALLELISM, and E2E_SEARCH_COUNT across all variants improves maintainability.
Also applies to: 240-243, 249-252, 258-261, 269-272, 279-282, 287-290, 296-299, 307-310, 316-319, 325-328, 334-337, 345-348, 355-358, 363-366, 372-375, 386-387, 393-394, 400-401
218-223: IndexInfo expectation switched to env-driven target — OKUsing value: E2E_EXPECTED_INDEX with retry_until_success_timeout should reduce flakes from delayed compaction.
482-484: Final IndexInfo empty-object check — OKThis matches the pattern used across the suite to validate terminal state.
410-413: No action required — E2E_UPDATE_COUNT does not exceed E2E_INSERT_COUNT in defaults/CIChecked Makefile and GitHub workflows: defaults are E2E_INSERT_COUNT=60000 and E2E_UPDATE_COUNT=10, and workflow overrides do not set UPDATE > INSERT (examples: .github/workflows/e2e.v2.yaml: INSERT=10000 UPDATE=100; .github/workflows/e2e.yaml: INSERT=10 UPDATE=10). Therefore the current config won't cause the not_found/already_exists churn.
Files inspected / locations:
- Makefile: E2E_INSERT_COUNT=60000, E2E_UPDATE_COUNT=10
- .github/workflows/* (several entries; no UPDATE > INSERT found)
- tests/v2/e2e/assets/multi_crud.yaml: lines 410–413 (also 437–440, 458–463) — no changes required.
211-213: Insert parameterization via envvars — VerifiedShort summary: loader and wiring are correct — no changes required.
- Placeholders used in tests/v2/e2e/assets:
- E2E_BULK_SIZE, E2E_DATASET_PATH, E2E_EXPECTED_INDEX, E2E_INSERT_COUNT, E2E_PARALLELISM, E2E_QPS, E2E_SEARCH_COUNT, E2E_TARGET_NAME, E2E_TARGET_NAMESPACE, E2E_UPDATE_COUNT
- Loader behavior:
- internal/config/config.go::GetActualValue (starts ~line 99) detects values wrapped with "_" (envSymbol), strips underscores, prepends "$" when needed, then calls os.ExpandEnv — it will therefore expand tokens like E2E_FOO at runtime. It also supports file://...
- Call sites:
- tests/v2/e2e/config/config.go invokes config.GetActualValue on loaded fields (e.g., around lines ~229, ~243, ~393–410), so the E2E_…_ tokens in the YAML assets will be resolved when the config is bound.
- Env provisioning:
- Makefile / Makefile.d define defaults and pass the E2E_* vars (e.g., E2E_INSERT_COUNT ?= 60000, E2E_PARALLELISM ?= 10, etc.) and Makefile.d/functions.mk forwards them.
- .github/workflows (e.g., .github/workflows/e2e.v2.yaml and others) export concrete E2E_* values used in CI (examples: E2E_PARALLELISM, E2E_INSERT_COUNT, E2E_QPS, E2E_EXPECTED_INDEX, E2E_SEARCH_COUNT, E2E_BULK_SIZE, E2E_UPDATE_COUNT, E2E_TARGET_NAMESPACE).
- Snippet (unchanged):
parallelism: _E2E_PARALLELISM_ num: _E2E_INSERT_COUNT_ qps: _E2E_QPS_Conclusion: substitution wiring is implemented and the referenced env names are present in Makefile/workflows — no further action required.
tests/v2/e2e/assets/index_correction.yaml (4)
24-24: Env-driven kubeconfig — OK_kube_config: KUBECONFIG is consistent with the unified configuration approach.
128-132: IndexProperty expect added — OKLowercased ge operator complies with our conventions. Keeping this improves CI feedback.
189-191: IndexInfo path/value expectations — OK
- Using path: $.sum() with value: E2E_EXPECTED_INDEX captures cluster-wide totals.
- The intermediate lt check after KillAgent is a good sanity assertion.
Also applies to: 210-213, 235-237
199-199: Added wait before KillAgent — sensible stabilizationwait: 10s before deleting the pod should reduce transient failures in subsequent IndexInfo.
tests/v2/e2e/assets/unary_crud.yaml (4)
24-24: Env-driven kubeconfig — OK_kube_config: KUBECONFIG matches the unified envvar strategy.
192-196: IndexProperty expect added — OKLowercased ge operator is correct. As noted in past learnings, this block is optional for local runs but useful for CI.
235-237: Search/Get/Exists/Timestamp parameterization — LGTMUniform use of E2E_PARALLELISM and E2E_SEARCH_COUNT across unary variants is consistent with the suite.
Also applies to: 243-245, 251-253, 259-261, 269-271, 277-279, 285-287, 293-295, 303-305, 311-313, 319-321, 327-329, 337-339, 345-347, 353-355, 361-363, 374-376, 381-383, 388-390
466-467: Final IndexInfo empty-object check — OKConsistent with the suite’s terminal validation.
Profile Report
|
Matts966
force-pushed
the
feature/unify-e2e-yaml
branch
from
cb4b8a3 to
c636ae0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
tests/v2/e2e/assets/index_correction.yaml (3)
189-191: Same concerns about $.sum() and E2E_EXPECTED_INDEX apply herePlease apply the same resolution as above or ensure both the evaluator and env exports are present.
210-213: LT check likely suffers from the same placeholder issueThe lt comparison is fine semantically post-agent-kill, but it still relies on E2E_EXPECTED_INDEX and possibly unsupported $.sum(). Address as per the earlier comment.
235-237: Final IndexInfo expect repeats the same risksLast consistency checkpoint still depends on $.sum() and E2E_EXPECTED_INDEX. Please align with the chosen fix.
🧹 Nitpick comments (3)
Makefile.d/helm.mk (1)
39-40: Apply the same retry policy to helm-docs download for consistencyThis mirrors the reliability improvement you added for Helm.
- && curl -fsSL "https://github.com/norwoodj/helm-docs/releases/download/v$(HELM_DOCS_VERSION)/$${TAR_NAME}" -o "$(TEMP_DIR)/$${TAR_NAME}"\ + && curl -fsSL --retry 3 "https://github.com/norwoodj/helm-docs/releases/download/v$(HELM_DOCS_VERSION)/$${TAR_NAME}" -o "$(TEMP_DIR)/$${TAR_NAME}"\tests/v2/e2e/assets/index_correction.yaml (1)
128-132: Consider making IndexProperty expect optional (local runs often don’t need it)Per our prior learning with these assets, the IndexProperty check tends to be flaky/unnecessary locally. Consider guarding this with an env flag (e.g., E2E_STRICT_SCHEMA_EXPECT) or remove for local mode.
If you want, I can add a conditional toggle to the loader to skip specific expects when an env is unset.
tests/v2/e2e/assets/unary_crud.yaml (1)
192-196: Make IndexProperty expect configurable to avoid flakiness locallyBased on prior experience, this expect is not required for local runs. Consider gating it behind an env flag or removing it in non-CI contexts.
Happy to wire an env toggle in the loader to skip this expect when not set.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
-
Makefile.d/helm.mk(1 hunks) -
tests/v2/e2e/assets/index_correction.yaml(7 hunks) -
tests/v2/e2e/assets/unary_crud.yaml(13 hunks) -
tests/v2/e2e/config/config.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/v2/e2e/config/config.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-04T06:38:31.386Z
Learnt from: Matts966
PR: vdaas/vald#3000
File: tests/v2/e2e/assets/index_correction.yaml:119-127
Timestamp: 2025-06-04T06:38:31.386Z
Learning: In tests/v2/e2e/assets/ configuration files, Index Property check operations do not require expect blocks for local testing scenarios.
Applied to files:
tests/v2/e2e/assets/index_correction.yamltests/v2/e2e/assets/unary_crud.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (170)
- GitHub Check: CodeQL Analyze (go)
- GitHub Check: CodeQL Analyze (actions)
- GitHub Check: CodeQL Analyze (rust)
- GitHub Check: check-format-diff
- GitHub Check: runner / go build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: CodeQL Analyze (go)
- GitHub Check: CodeQL Analyze (actions)
- GitHub Check: CodeQL Analyze (rust)
- GitHub Check: check-format-diff
- GitHub Check: runner / go build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: CodeQL Analyze (go)
- GitHub Check: CodeQL Analyze (actions)
- GitHub Check: CodeQL Analyze (rust)
- GitHub Check: check-format-diff
- GitHub Check: runner / go build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: CodeQL Analyze (go)
- GitHub Check: CodeQL Analyze (actions)
- GitHub Check: CodeQL Analyze (rust)
- GitHub Check: check-format-diff
- GitHub Check: runner / go build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: CodeQL Analyze (go)
- GitHub Check: CodeQL Analyze (actions)
- GitHub Check: CodeQL Analyze (rust)
- GitHub Check: check-format-diff
- GitHub Check: runner / go build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: CodeQL Analyze (go)
- GitHub Check: CodeQL Analyze (actions)
- GitHub Check: CodeQL Analyze (rust)
- GitHub Check: check-format-diff
- GitHub Check: runner / go build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: CodeQL Analyze (go)
- GitHub Check: CodeQL Analyze (actions)
- GitHub Check: CodeQL Analyze (rust)
- GitHub Check: check-format-diff
- GitHub Check: runner / go build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: CodeQL Analyze (go)
- GitHub Check: CodeQL Analyze (actions)
- GitHub Check: CodeQL Analyze (rust)
- GitHub Check: check-format-diff
- GitHub Check: runner / go build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: CodeQL Analyze (go)
- GitHub Check: CodeQL Analyze (actions)
- GitHub Check: CodeQL Analyze (rust)
- GitHub Check: check-format-diff
- GitHub Check: runner / go build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: CodeQL Analyze (go)
- GitHub Check: CodeQL Analyze (actions)
- GitHub Check: CodeQL Analyze (rust)
- GitHub Check: check-format-diff
- GitHub Check: runner / go build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
🔇 Additional comments (13)
Makefile.d/helm.mk (2)
26-26: Good call adding curl retries for Helm downloadAdding --retry 3 will reduce flakiness from transient network issues.
21-28: TEMP_DIR is created in the top-level Makefile — no change requiredMakefile defines and creates TEMP_DIR via mktemp (Makefile:68), so the cd into $(TEMP_DIR) in Makefile.d/helm.mk is safe; several other rules already rely on that directory.
- Makefile:68 — TEMP_DIR :=
$(eval TEMP_DIR := $ (shell mktemp -d))$(TEMP_DIR)- Example usages that assume TEMP_DIR exists:
- Makefile.d/tools.mk:190 (mkdir -p $(TEMP_DIR)/go)
- Makefile.d/tools.mk:210 (mkdir -p $(TEMP_DIR)/zlib)
- Makefile.d/kind.mk:87 (mkdir -p $(TEMP_DIR)/csi-driver-hostpath)
tests/v2/e2e/assets/index_correction.yaml (3)
24-24: Verify KUBECONFIG substitution is wired end-to-endGood move to parameterize kube_config. Ensure the config loader expands KUBECONFIG and that your Makefile/GHA export it in all entry points running this asset. Paths containing ~ won’t expand unless the loader handles shell expansion explicitly; prefer absolute paths.
Would you like me to scan the repo to confirm all entry points export KUBECONFIG and the loader performs env substitution?
150-158: Env-parametrization of Insert and stabilization retry look goodUsing E2E_PARALLELISM, E2E_INSERT_COUNT, E2E_QPS and adding retry_until_success_timeout improves reuse and stability.
160-161: Action needed: confirm JSONPath sum() support or change the expectation placeholderShort summary (verified):
- E2E_EXPECTED_INDEX is defined/exported: Makefile sets a default and .github/workflows/e2e.v2.yaml overrides it.
- The test assets use $.sum() in multiple places (tests/v2/e2e/assets/index_correction.yaml and readreplica.yaml), so the JSONPath function concern is valid and must be confirmed.
Files that need attention:
- tests/v2/e2e/assets/index_correction.yaml (paths at lines ~160, 189, 210, 235)
- tests/v2/e2e/assets/readreplica.yaml (path at ~431)
- Other assets referencing E2E_EXPECTED_INDEX: stream_crud.yaml, unary_crud.yaml, rollout.yaml, multi_crud.yaml (as found by the search)
Recommended actions (pick one):
- Verify the test runner’s JSONPath evaluator supports the sum() function. If it does, no change needed for the path.
- If sum() is unsupported, replace path/value pairs to use a supported field (for example, use $.stored and compare against E2E_INSERT_COUNT) or implement aggregation in the test harness.
Suggested minimal diff if sum() is unsupported:
path: $.sum()value: _E2E_EXPECTED_INDEX_
path: $.storedvalue: _E2E_INSERT_COUNT_tests/v2/e2e/assets/unary_crud.yaml (8)
24-24: Confirm KUBECONFIG is propagated in all execution pathsNow that kube_config is parameterized, ensure both CI and local invocations export KUBECONFIG (absolute path recommended) and that the loader expands it. Otherwise port-forwarding will fail early.
I can add a quick verification script to enumerate where it’s set if useful.
215-218: Good: insert parameters are env-drivenSwitching to E2E_PARALLELISM, E2E_INSERT_COUNT, and E2E_QPS increases reuse and makes CI/tests share the same asset cleanly.
374-389: Search-related ops parameterization looks consistentParallelism and num use the expected placeholders across GetObject/Exists/Timestamp. Good consistency with the Makefile/workflow vars.
398-401: Explicitly resetting offset to 0 is helpfulOffset=0 prevents carry-over between runs; good for deterministic updates.
413-415: Env-var coverage for remove counts looks correctAligns with E2E_UPDATE_COUNT as used elsewhere in this asset.
443-447: Upsert parameters are consistent with the rest of the suiteReusing E2E_PARALLELISM and E2E_UPDATE_COUNT is consistent and reduces duplication.
466-468: Final IndexInfo expect for {} matches existing conventionsThe trailing sanity check remains consistent with the suite’s pattern.
222-227: E2E_EXPECTED_INDEX is already defined in CI/Makefile / workflows — changing it is optionalShort: E2E_EXPECTED_INDEX is defined/exported in the repo’s Makefile and set in CI workflows, so keeping it is safe. Replacing it with E2E_INSERT_COUNT is a valid simplification but not required.
Attention (locations):
- usages (placeholders in e2e assets)
- tests/v2/e2e/assets/unary_crud.yaml — expect: $.stored value: E2E_EXPECTED_INDEX
- tests/v2/e2e/assets/stream_crud.yaml — expect: $.stored value: E2E_EXPECTED_INDEX
- tests/v2/e2e/assets/multi_crud.yaml — expect: $.stored value: E2E_EXPECTED_INDEX
- tests/v2/e2e/assets/index_correction.yaml (several expect entries using E2E_EXPECTED_INDEX)
- tests/v2/e2e/assets/rollout.yaml (uses E2E_EXPECTED_INDEX)
- where the env vars are defined / set
- Makefile — E2E_INSERT_COUNT and E2E_EXPECTED_INDEX are defined (Makefile)
- .github/workflows/e2e.v2.yaml — E2E_INSERT_COUNT / E2E_EXPECTED_INDEX are set for CI
Optional diff (apply only if you want expected==inserted):
expect: - status_code: ok path: $.stored - value: _E2E_EXPECTED_INDEX_ + value: _E2E_INSERT_COUNT_Recommendation: leave as-is if you intentionally need a separate expected value; otherwise you can apply the diff above to derive the expectation from the insert count.
Likely an incorrect or invalid review comment.
Profile Report
|
Description
Unified E2E V2 YAML setttings using envvars
Related Issue
Resolve #3111
Versions
Checklist
Special notes for your reviewer
Summary by CodeRabbit
Tests
Chores
Developer experience