ECOPROJECT-4214 | feat: persist cluster requirements inputs with upsert and add GET endpoint for PDF reuse#1061
Open
ronenav wants to merge 1 commit intokubev2v:mainfrom
Open
ECOPROJECT-4214 | feat: persist cluster requirements inputs with upsert and add GET endpoint for PDF reuse#1061ronenav wants to merge 1 commit intokubev2v:mainfrom
ronenav wants to merge 1 commit intokubev2v:mainfrom
Conversation
ronenav
requested review from
AvielSegev and
nirarg
and removed request for
a team
|
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:
📝 WalkthroughWalkthroughAdds a new GET endpoint to retrieve persisted cluster sizing inputs, a persisted stored-input model and DB migration, store bindings, service/handler/mappers updates for persistence and auth, generated client/server bindings, and corresponding tests and mocks. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Handler
participant Auth
participant Sizer
participant Store
participant DB
Client->>Handler: GET /api/v1/assessments/{id}/cluster-requirements?clusterId=cid
Handler->>Auth: authenticate & extract user
Auth-->>Handler: user context
Handler->>Sizer: GetClusterRequirementsInput(assessmentID, clusterId)
Sizer->>Auth: verify assessment ownership
Auth-->>Sizer: authorized / forbidden
alt authorized
Sizer->>Store: Get(ctx, assessmentID, clusterId)
Store->>DB: SELECT FROM assessment_cluster_sizing_inputs
DB-->>Store: record / not found
alt record found
Store-->>Sizer: AssessmentClusterSizingInput
Sizer->>Handler: mapped ClusterRequirementsStoredInput
Handler-->>Client: 200 + JSON
else not found
Store-->>Sizer: ErrRecordNotFound
Sizer-->>Handler: 404
Handler-->>Client: 404
end
else forbidden
Sizer-->>Handler: 403
Handler-->>Client: 403
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
ronenav
force-pushed
the
4214-sizer-to-db
branch
from
57fe6ca to
b1e8cfa
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/api/client/client.gen.go`:
- Around line 1114-1130: The GET request builder currently proceeds when params
is nil and may omit the required clusterId query param; update the code that
handles params (the block using params, queryURL, queryValues and
runtime.StyleParamWithLocation("form", true, "clusterId", ...)) to validate that
params != nil and params.ClusterId is set (non-empty) before attempting to build
the query string, and return an appropriate error if clusterId is missing so
callers fail fast rather than emitting an invalid request to the server.
In `@internal/handlers/v1alpha1/sizer_test.go`:
- Around line 1350-1421: Add a test that exercises
handler.GetAssessmentClusterRequirements when the assessment itself is missing:
create a new non-existent UUID (e.g., nonExistentID := uuid.New()), call
handler.GetAssessmentClusterRequirements with
server.GetAssessmentClusterRequirementsRequestObject{Id: nonExistentID, Params:
api.GetAssessmentClusterRequirementsParams{ClusterId: clusterID}}, and assert
the response is a server.GetAssessmentClusterRequirements404JSONResponse; place
this alongside the other It blocks in the
Describe("GetAssessmentClusterRequirements") block so it verifies 404 for a
missing assessment.
In `@internal/handlers/v1alpha1/sizer.go`:
- Around line 33-40: The switch on err.(type) in the
GetAssessmentClusterRequirements handler currently maps only
*service.ErrResourceNotFound to 404 and falls through to 500 for all other
errors; add a case for *service.ErrForbidden (the same error type handled in
CalculateAssessmentClusterRequirements) and return
server.GetAssessmentClusterRequirements403JSONResponse{Message: err.Error()},
while keeping the existing logger.Error(...).WithUUID("assessment_id",
request.Id).Log() behavior; update the switch in
internal/handlers/v1alpha1/sizer.go (the handler that returns
GetAssessmentClusterRequirements404JSONResponse/500JSONResponse) to include this
new case.
In `@internal/service/sizer_test.go`:
- Around line 1435-1439: The test sets request.WorkerNodeThreads =
util.IntPtr(0), which is invalid for the public API; replace that explicit zero
with nil to represent “not provided” (i.e., remove the util.IntPtr(0) assignment
and set request.WorkerNodeThreads = nil) so the test uses API-valid input—update
the It(...) case in sizer_test.go where request is configured and any helper
usage of WorkerNodeThreads in that test path accordingly.
In `@internal/store/cluster_sizing_input_test.go`:
- Around line 78-102: The test suite is missing a case that verifies
ClusterSizingInput().Get returns store.ErrRecordNotFound when no record exists;
add a new It block in internal/store/cluster_sizing_input_test.go that calls
s.ClusterSizingInput().Get with a fresh assessmentID and a non-existent
ExternalClusterID and assert the returned error equals store.ErrRecordNotFound
(use Expect(err).To(Equal(store.ErrRecordNotFound))). Ensure the test does not
insert any records for that assessmentID so the Get call hits the not-found
path.
- Around line 62-76: The Upsert currently uses GORM's AssignmentColumns which
writes struct fields (including nils) on conflict, so passing a partially
populated model.AssessmentClusterSizingInput causes existing columns
(CpuOverCommitRatio, MemoryOverCommitRatio) to be nulled; update the
ClusterSizingInput().Upsert implementation to perform a partial-update instead:
detect nil pointer fields on the incoming AssessmentClusterSizingInput and only
include non-nil fields in the conflict update (e.g., build a map of non-nil
column->value or use GORM's Updates/Select to update only present fields) so
that nils do not overwrite existing DB values, or alternatively add explicit
docs and tests if the intended semantics are to allow nils to clear columns;
ensure the change touches the Upsert function and any code paths that call
AssignmentColumns so the test behavior aligns with intended semantics.
In `@pkg/migrations/sql/20260326150000_assessment_cluster_sizing_inputs.sql`:
- Around line 6-17: This migration must add DB-level CHECK constraints for the
sizing fields to prevent out-of-range values: change the cpu_over_commit_ratio
and memory_over_commit_ratio columns from unconstrained TEXT to either a numeric
type with a range CHECK or a constrained TEXT with an IN(...) CHECK (pick the
allowed ratio set), and add CHECK constraints on worker_node_cpu,
worker_node_threads, worker_node_memory, control_plane_node_count,
control_plane_cpu, and control_plane_memory to enforce minimum/maximum bounds
(e.g. >=1 and <= reasonable upper limits) and keep hosted_control_plane as
BOOLEAN; add these CHECK clauses directly in the CREATE TABLE statement (or via
ALTER TABLE in this migration) referencing the exact column names
(cpu_over_commit_ratio, memory_over_commit_ratio, worker_node_cpu,
worker_node_threads, worker_node_memory, control_plane_node_count,
control_plane_cpu, control_plane_memory, hosted_control_plane) so the schema
rejects invalid values at insert/update time.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: ad99f12c-f583-47c5-b7d7-8a529271b454
📒 Files selected for processing (18)
api/v1alpha1/openapi.yamlapi/v1alpha1/spec.gen.goapi/v1alpha1/types.gen.gointernal/api/client/client.gen.gointernal/api/server/server.gen.gointernal/handlers/v1alpha1/mappers/inbound.gointernal/handlers/v1alpha1/mappers/outbound.gointernal/handlers/v1alpha1/sizer.gointernal/handlers/v1alpha1/sizer_test.gointernal/service/errors.gointernal/service/mappers/inbound.gointernal/service/sizer.gointernal/service/sizer_test.gointernal/store/cluster_sizing_input.gointernal/store/cluster_sizing_input_test.gointernal/store/model/cluster_sizing_input.gointernal/store/store.gopkg/migrations/sql/20260326150000_assessment_cluster_sizing_inputs.sql
There was a problem hiding this comment.
♻️ Duplicate comments (4)
internal/handlers/v1alpha1/sizer_test.go (1)
1350-1421:⚠️ Potential issue | 🟡 MinorAdd a GET test for non-existent assessment (404).
Current cases validate missing stored input for an existing assessment, but not a missing assessment itself. Add an explicit 404 assertion for that path to prevent regression in error mapping.
💡 Suggested test addition
+ It("returns 404 when assessment does not exist", func() { + nonExistentID := uuid.New() + resp, err := handler.GetAssessmentClusterRequirements(ctx, server.GetAssessmentClusterRequirementsRequestObject{ + Id: nonExistentID, + Params: api.GetAssessmentClusterRequirementsParams{ + ClusterId: clusterID, + }, + }) + Expect(err).To(BeNil()) + _, ok := resp.(server.GetAssessmentClusterRequirements404JSONResponse) + Expect(ok).To(BeTrue()) + })As per coding guidelines:
**/*_test.gorequires high coverage on critical logic paths.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/handlers/v1alpha1/sizer_test.go` around lines 1350 - 1421, Add a test inside the Describe("GetAssessmentClusterRequirements") suite that verifies a missing assessment (not just missing stored cluster input) returns 404: ensure mockStore.assessments does NOT contain assessmentID (or explicitly delete it), call handler.GetAssessmentClusterRequirements with Id: assessmentID and Params.ClusterId: clusterID, then assert err is nil and the response is of type server.GetAssessmentClusterRequirements404JSONResponse; reference the existing handler.GetAssessmentClusterRequirements, mockStore.assessments and assessmentID to locate where to add this test.internal/service/sizer_test.go (1)
1435-1439:⚠️ Potential issue | 🟡 MinorUse omission (
nil) instead of explicit zero forWorkerNodeThreadsin this test.Line 1438 uses
util.IntPtr(0), which does not represent a valid explicit API input and weakens the intended omitted-vs-provided semantics in this scenario.🔧 Suggested fix
- request.WorkerNodeThreads = util.IntPtr(0) + request.WorkerNodeThreads = nilAs per coding guidelines:
**/*_test.goexpects high-quality tests that clearly reflect intended behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/service/sizer_test.go` around lines 1435 - 1439, The test "detects violation even with many small batches concentrated on one node" currently sets request.WorkerNodeThreads = util.IntPtr(0) which incorrectly models an explicit zero rather than an omitted value; change this to set request.WorkerNodeThreads = nil so the test exercises the omitted-vs-provided semantics correctly (locate the assignment to WorkerNodeThreads in that test and replace util.IntPtr(0) with nil).pkg/migrations/sql/20260326150000_assessment_cluster_sizing_inputs.sql (1)
3-17:⚠️ Potential issue | 🟠 MajorAdd DB CHECK constraints for enum/bounded sizing fields.
The table currently accepts out-of-contract values (ratios, node counts, thread bounds). Add schema constraints so invalid data cannot be persisted through non-API paths.
🛡️ Suggested migration hardening
CREATE TABLE IF NOT EXISTS assessment_cluster_sizing_inputs ( assessment_id VARCHAR(255) NOT NULL REFERENCES assessments(id) ON DELETE CASCADE, external_cluster_id TEXT NOT NULL, cpu_over_commit_ratio TEXT, memory_over_commit_ratio TEXT, worker_node_cpu INTEGER, worker_node_threads INTEGER, worker_node_memory INTEGER, control_plane_schedulable BOOLEAN, control_plane_node_count INTEGER, control_plane_cpu INTEGER, control_plane_memory INTEGER, hosted_control_plane BOOLEAN, updated_at TIMESTAMPTZ NOT NULL DEFAULT now(), + CHECK (cpu_over_commit_ratio IS NULL OR cpu_over_commit_ratio IN ('1:1','1:2','1:4','1:6')), + CHECK (memory_over_commit_ratio IS NULL OR memory_over_commit_ratio IN ('1:1','1:2','1:4')), + CHECK (worker_node_cpu IS NULL OR worker_node_cpu > 0), + CHECK (worker_node_memory IS NULL OR worker_node_memory > 0), + CHECK (control_plane_node_count IS NULL OR control_plane_node_count IN (1,3)), + CHECK (control_plane_cpu IS NULL OR control_plane_cpu > 0), + CHECK (control_plane_memory IS NULL OR control_plane_memory > 0), + CHECK (worker_node_threads IS NULL OR worker_node_threads BETWEEN 2 AND 2000), + CHECK (worker_node_threads IS NULL OR worker_node_cpu IS NULL OR worker_node_threads >= worker_node_cpu), PRIMARY KEY (assessment_id, external_cluster_id) );Based on learnings: the project intentionally uses a
workerNodeThreadsmaximum of 2000 and expects schema/API alignment.
As per coding guidelines: API/input validation is mandatory.internal/api/client/client.gen.go (1)
1089-1130:⚠️ Potential issue | 🟠 MajorFail fast when required
clusterIdis missing.At Line 1114, request construction still allows
params == nil, and an emptyparams.ClusterIdis not rejected. This can emit an invalid request missing a required query param.Proposed fix
func NewGetAssessmentClusterRequirementsRequest(server string, id openapi_types.UUID, params *GetAssessmentClusterRequirementsParams) (*http.Request, error) { var err error + if params == nil || params.ClusterId == "" { + return nil, fmt.Errorf("missing required query parameter: clusterId") + } @@ - if params != nil { - queryValues := queryURL.Query() + queryValues := queryURL.Query() - if queryFrag, err := runtime.StyleParamWithLocation("form", true, "clusterId", runtime.ParamLocationQuery, params.ClusterId); err != nil { - return nil, err - } else if parsed, err := url.ParseQuery(queryFrag); err != nil { - return nil, err - } else { - for k, v := range parsed { - for _, v2 := range v { - queryValues.Add(k, v2) - } + if queryFrag, err := runtime.StyleParamWithLocation("form", true, "clusterId", runtime.ParamLocationQuery, params.ClusterId); err != nil { + return nil, err + } else if parsed, err := url.ParseQuery(queryFrag); err != nil { + return nil, err + } else { + for k, v := range parsed { + for _, v2 := range v { + queryValues.Add(k, v2) } } - - queryURL.RawQuery = queryValues.Encode() } + queryURL.RawQuery = queryValues.Encode()As per coding guidelines, “API Design: APIs (REST, gRPC) must be well-defined, versioned, and backward-compatible. Input validation is mandatory.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/client/client.gen.go` around lines 1089 - 1130, The request builder NewGetAssessmentClusterRequirementsRequest currently allows params == nil or params.ClusterId empty, producing requests missing a required query parameter; update NewGetAssessmentClusterRequirementsRequest to validate that params is non-nil and params.ClusterId is set (non-empty/zero) before building the URL and if missing return an error immediately (e.g., a descriptive error indicating missing required parameter "clusterId"); reference GetAssessmentClusterRequirementsParams.ClusterId and ensure the check is placed before the query construction block so the function fails fast.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@internal/api/client/client.gen.go`:
- Around line 1089-1130: The request builder
NewGetAssessmentClusterRequirementsRequest currently allows params == nil or
params.ClusterId empty, producing requests missing a required query parameter;
update NewGetAssessmentClusterRequirementsRequest to validate that params is
non-nil and params.ClusterId is set (non-empty/zero) before building the URL and
if missing return an error immediately (e.g., a descriptive error indicating
missing required parameter "clusterId"); reference
GetAssessmentClusterRequirementsParams.ClusterId and ensure the check is placed
before the query construction block so the function fails fast.
In `@internal/handlers/v1alpha1/sizer_test.go`:
- Around line 1350-1421: Add a test inside the
Describe("GetAssessmentClusterRequirements") suite that verifies a missing
assessment (not just missing stored cluster input) returns 404: ensure
mockStore.assessments does NOT contain assessmentID (or explicitly delete it),
call handler.GetAssessmentClusterRequirements with Id: assessmentID and
Params.ClusterId: clusterID, then assert err is nil and the response is of type
server.GetAssessmentClusterRequirements404JSONResponse; reference the existing
handler.GetAssessmentClusterRequirements, mockStore.assessments and assessmentID
to locate where to add this test.
In `@internal/service/sizer_test.go`:
- Around line 1435-1439: The test "detects violation even with many small
batches concentrated on one node" currently sets request.WorkerNodeThreads =
util.IntPtr(0) which incorrectly models an explicit zero rather than an omitted
value; change this to set request.WorkerNodeThreads = nil so the test exercises
the omitted-vs-provided semantics correctly (locate the assignment to
WorkerNodeThreads in that test and replace util.IntPtr(0) with nil).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6b15d080-d1c4-4b08-b581-f6201f5a628d
📒 Files selected for processing (18)
api/v1alpha1/openapi.yamlapi/v1alpha1/spec.gen.goapi/v1alpha1/types.gen.gointernal/api/client/client.gen.gointernal/api/server/server.gen.gointernal/handlers/v1alpha1/mappers/inbound.gointernal/handlers/v1alpha1/mappers/outbound.gointernal/handlers/v1alpha1/sizer.gointernal/handlers/v1alpha1/sizer_test.gointernal/service/errors.gointernal/service/mappers/inbound.gointernal/service/sizer.gointernal/service/sizer_test.gointernal/store/cluster_sizing_input.gointernal/store/cluster_sizing_input_test.gointernal/store/model/cluster_sizing_input.gointernal/store/store.gopkg/migrations/sql/20260326150000_assessment_cluster_sizing_inputs.sql
👮 Files not reviewed due to content moderation or server errors (8)
- api/v1alpha1/spec.gen.go
- internal/service/errors.go
- internal/store/store.go
- internal/handlers/v1alpha1/mappers/inbound.go
- internal/handlers/v1alpha1/mappers/outbound.go
- internal/handlers/v1alpha1/sizer.go
- internal/store/cluster_sizing_input_test.go
- api/v1alpha1/openapi.yaml
ronenav
force-pushed
the
4214-sizer-to-db
branch
from
b1e8cfa to
6cd1a7f
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
internal/api/client/client.gen.go (1)
1088-1138:⚠️ Potential issue | 🟠 MajorValidate required
clusterIdbefore building the request.Line 1114 allows
params == nil, andparams.ClusterIdis not checked for empty value, so callers can emit an invalid request and fail only at runtime.Proposed fix
func NewGetAssessmentClusterRequirementsRequest(server string, id openapi_types.UUID, params *GetAssessmentClusterRequirementsParams) (*http.Request, error) { var err error + if params == nil || strings.TrimSpace(params.ClusterId) == "" { + return nil, fmt.Errorf("missing required query parameter: clusterId") + } @@ - if params != nil { - queryValues := queryURL.Query() - - if queryFrag, err := runtime.StyleParamWithLocation("form", true, "clusterId", runtime.ParamLocationQuery, params.ClusterId); err != nil { - return nil, err - } else if parsed, err := url.ParseQuery(queryFrag); err != nil { - return nil, err - } else { - for k, v := range parsed { - for _, v2 := range v { - queryValues.Add(k, v2) - } - } - } - - queryURL.RawQuery = queryValues.Encode() + queryValues := queryURL.Query() + if queryFrag, err := runtime.StyleParamWithLocation("form", true, "clusterId", runtime.ParamLocationQuery, params.ClusterId); err != nil { + return nil, err + } else if parsed, err := url.ParseQuery(queryFrag); err != nil { + return nil, err + } else { + for k, v := range parsed { + for _, v2 := range v { + queryValues.Add(k, v2) + } + } } + queryURL.RawQuery = queryValues.Encode()As per coding guidelines, “API Design: APIs (REST, gRPC) must be well-defined, versioned, and backward-compatible. Input validation is mandatory.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/client/client.gen.go` around lines 1088 - 1138, The request builder NewGetAssessmentClusterRequirementsRequest must validate that params is non-nil and that the required params.ClusterId is present/non-empty before constructing the query URL; update NewGetAssessmentClusterRequirementsRequest to check if params == nil or params.ClusterId is empty and return a clear error (e.g., fmt.Errorf("clusterId is required")) instead of proceeding, so callers get a fast, descriptive validation error; refer to the GetAssessmentClusterRequirementsParams type and the params.ClusterId field and perform this check prior to the block that builds queryValues and calls runtime.StyleParamWithLocation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/v1alpha1/openapi.yaml`:
- Around line 2533-2535: ClusterRequirementsStoredInput.workerNodeThreads is
missing the same bounds as ClusterRequirementsRequest.workerNodeThreads; update
the OpenAPI schema entry for ClusterRequirementsStoredInput.workerNodeThreads to
include minimum: 2 and maximum: 2000 (matching the documented bounds for
ClusterRequirementsRequest.workerNodeThreads) so both schemas remain consistent
for client reuse.
In `@internal/store/cluster_sizing_input_test.go`:
- Around line 35-38: The AfterEach cleanup currently calls gormdb.Exec("DELETE
FROM assessment_cluster_sizing_inputs;") and gormdb.Exec("DELETE FROM
assessments;") without checking for errors; update the AfterEach to capture each
Exec result (from gormdb.Exec(...)) and assert that result.Error is nil (e.g.,
using Expect(result.Error).NotTo(HaveOccurred()) or a test failure helper) so
any cleanup failure is surfaced and fails the test; reference AfterEach and
gormdb.Exec calls and the table names assessment_cluster_sizing_inputs and
assessments when making the change.
In `@pkg/migrations/migrations_test.go`:
- Line 65: The migration test currently drops assessment_cluster_sizing_inputs
in teardown but never asserts the migration actually created it; update the test
(the function using gormdb in migrations_test.go) to assert the table exists
after running the migration by calling
gormdb.Migrator().HasTable("assessment_cluster_sizing_inputs") (or executing a
simple SELECT 1 FROM assessment_cluster_sizing_inputs LIMIT 1 and checking no
error) and require the result to be true before the teardown drop; keep the
existing gormdb variable and teardown line intact.
---
Duplicate comments:
In `@internal/api/client/client.gen.go`:
- Around line 1088-1138: The request builder
NewGetAssessmentClusterRequirementsRequest must validate that params is non-nil
and that the required params.ClusterId is present/non-empty before constructing
the query URL; update NewGetAssessmentClusterRequirementsRequest to check if
params == nil or params.ClusterId is empty and return a clear error (e.g.,
fmt.Errorf("clusterId is required")) instead of proceeding, so callers get a
fast, descriptive validation error; refer to the
GetAssessmentClusterRequirementsParams type and the params.ClusterId field and
perform this check prior to the block that builds queryValues and calls
runtime.StyleParamWithLocation.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 0d9e235b-ba06-4fec-baa9-de6de9bd8276
📒 Files selected for processing (19)
api/v1alpha1/openapi.yamlapi/v1alpha1/spec.gen.goapi/v1alpha1/types.gen.gointernal/api/client/client.gen.gointernal/api/server/server.gen.gointernal/handlers/v1alpha1/mappers/inbound.gointernal/handlers/v1alpha1/mappers/outbound.gointernal/handlers/v1alpha1/sizer.gointernal/handlers/v1alpha1/sizer_test.gointernal/service/errors.gointernal/service/mappers/inbound.gointernal/service/sizer.gointernal/service/sizer_test.gointernal/store/cluster_sizing_input.gointernal/store/cluster_sizing_input_test.gointernal/store/model/cluster_sizing_input.gointernal/store/store.gopkg/migrations/migrations_test.gopkg/migrations/sql/20260326150000_assessment_cluster_sizing_inputs.sql
ronenav
force-pushed
the
4214-sizer-to-db
branch
from
6cd1a7f to
c12e557
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
internal/handlers/v1alpha1/sizer.go (1)
31-40:⚠️ Potential issue | 🟠 MajorReturn 403 when
GetAssessmentdenies access.
GetAssessmentcan still return*service.ErrForbidden; this branch currently falls through to 500. As per coding guidelines: Security requires guarding against authorization flaws, including correct authorization handling.🔧 Suggested fix
if err != nil { switch err.(type) { case *service.ErrResourceNotFound: logger.Error(err).WithUUID("assessment_id", request.Id).Log() return server.GetAssessmentClusterRequirements404JSONResponse{Message: err.Error()}, nil + case *service.ErrForbidden: + logger.Error(err).WithUUID("assessment_id", request.Id).Log() + return server.GetAssessmentClusterRequirements403JSONResponse{Message: err.Error()}, nil default: logger.Error(err).WithUUID("assessment_id", request.Id).Log() return server.GetAssessmentClusterRequirements500JSONResponse{Message: fmt.Sprintf("failed to get assessment: %v", err)}, nil } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/handlers/v1alpha1/sizer.go` around lines 31 - 40, The error handling for h.assessmentSrv.GetAssessment currently maps ErrResourceNotFound to 404 and everything else to 500; update the switch to handle *service.ErrForbidden explicitly by logging the error (use existing logger.Error(err).WithUUID("assessment_id", request.Id).Log()) and returning server.GetAssessmentClusterRequirements403JSONResponse with an appropriate message; keep the existing 404 and 500 branches for *service.ErrResourceNotFound and default respectively so unauthorized access is returned as 403 instead of 500.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/handlers/v1alpha1/sizer.go`:
- Around line 29-31: Reject empty clusterId early: check
request.Params.ClusterId (clusterID) immediately after reading it and before
calling h.assessmentSrv.GetAssessment; if clusterID == "" return a
400/bad-request response/error instead of proceeding (so the input error is
surfaced rather than turning into a 404 from GetAssessment). Ensure the
validation happens in the same handler where clusterID is read (near the lines
using clusterID, request.Params.ClusterId, auth.MustHaveUser(ctx), and before
calling h.assessmentSrv.GetAssessment).
In `@internal/service/sizer_test.go`:
- Around line 1021-1037: Add a second-call assertion to verify upsert behavior:
after the first CalculateClusterRequirements call and assertions, invoke
sizerService.CalculateClusterRequirements(ctx, assessmentID, request) again
using a modified response (e.g. createTestSizerResponse with different numeric
values and omit some optional fields) or a changed request, then call
sizerService.GetClusterRequirementsInput(ctx, assessmentID, clusterID) and
assert that the returned storedInput reflects the new values (e.g. updated
WorkerNodeCPU and other changed fields) to ensure the previous row was
overwritten; use the existing test helpers (createTestSizerResponse, testServer,
sizerClient, mockStore, assessmentID, clusterID, request) and keep error checks
(Expect(err).To(BeNil())) for both calls.
In `@internal/store/cluster_sizing_input.go`:
- Around line 34-75: The Upsert and Get methods call getDB(ctx) but do not chain
WithContext(ctx), so GORM won't respect the request context; update both methods
to call getDB(ctx).WithContext(ctx) before further chaining (i.e., in Upsert
before .Clauses(...).Create(&input) and in Get before .First(&input, ...)) so
the DB operations honor cancellations and timeouts.
---
Duplicate comments:
In `@internal/handlers/v1alpha1/sizer.go`:
- Around line 31-40: The error handling for h.assessmentSrv.GetAssessment
currently maps ErrResourceNotFound to 404 and everything else to 500; update the
switch to handle *service.ErrForbidden explicitly by logging the error (use
existing logger.Error(err).WithUUID("assessment_id", request.Id).Log()) and
returning server.GetAssessmentClusterRequirements403JSONResponse with an
appropriate message; keep the existing 404 and 500 branches for
*service.ErrResourceNotFound and default respectively so unauthorized access is
returned as 403 instead of 500.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 255ab0e9-7346-425d-84d7-d350d9ec4c8c
📒 Files selected for processing (19)
api/v1alpha1/openapi.yamlapi/v1alpha1/spec.gen.goapi/v1alpha1/types.gen.gointernal/api/client/client.gen.gointernal/api/server/server.gen.gointernal/handlers/v1alpha1/mappers/inbound.gointernal/handlers/v1alpha1/mappers/outbound.gointernal/handlers/v1alpha1/sizer.gointernal/handlers/v1alpha1/sizer_test.gointernal/service/errors.gointernal/service/mappers/inbound.gointernal/service/sizer.gointernal/service/sizer_test.gointernal/store/cluster_sizing_input.gointernal/store/cluster_sizing_input_test.gointernal/store/model/cluster_sizing_input.gointernal/store/store.gopkg/migrations/migrations_test.gopkg/migrations/sql/20260326150000_assessment_cluster_sizing_inputs.sql
ronenav
force-pushed
the
4214-sizer-to-db
branch
from
c12e557 to
5de39a8
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
internal/handlers/v1alpha1/sizer.go (1)
38-47:⚠️ Potential issue | 🟠 MajorMissing
ErrForbiddencase in error handling.The
GetAssessmentcall can return*service.ErrForbidden, which is handled inCalculateAssessmentClusterRequirements(line 161-163) but missing here. This causes forbidden errors to fall through to a 500 response instead of 403.🔧 Suggested fix
if err != nil { switch err.(type) { case *service.ErrResourceNotFound: logger.Error(err).WithUUID("assessment_id", request.Id).Log() return server.GetAssessmentClusterRequirements404JSONResponse{Message: err.Error()}, nil + case *service.ErrForbidden: + logger.Error(err).WithUUID("assessment_id", request.Id).Log() + return server.GetAssessmentClusterRequirements403JSONResponse{Message: err.Error()}, nil default: logger.Error(err).WithUUID("assessment_id", request.Id).Log() return server.GetAssessmentClusterRequirements500JSONResponse{Message: fmt.Sprintf("failed to get assessment: %v", err)}, nil } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/handlers/v1alpha1/sizer.go` around lines 38 - 47, The error handling block after GetAssessment is missing a case for *service.ErrForbidden so forbidden errors are treated as 500; add a case for *service.ErrForbidden in the switch that logs the error with logger.Error(err).WithUUID("assessment_id", request.Id).Log() and returns server.GetAssessmentClusterRequirements403JSONResponse{Message: err.Error()}, nil (mirroring the CalculateAssessmentClusterRequirements handling), leaving the existing ErrResourceNotFound and default branches unchanged.internal/api/client/client.gen.go (1)
1114-1130:⚠️ Potential issue | 🟠 MajorFail fast when
clusterIdis missing.Line 1114 still permits
params == nil, and Line 1117 builds the query fromparams.ClusterIdwithout rejecting its zero value. That means this client can emit/api/v1/assessments/{id}/cluster-requirementswithout the requiredclusterIdand only fail at server runtime. Since this file is generated, please fix the OpenAPI source / generation input so the builder rejects missingclusterIdbefore issuing the request.As per coding guidelines, “API Design: APIs (REST, gRPC) must be well-defined, versioned, and backward-compatible. Input validation is mandatory.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/client/client.gen.go` around lines 1114 - 1130, The code currently allows params to be nil or params.ClusterId to be empty and proceeds to build the query; add an early validation that rejects missing clusterId before calling runtime.StyleParamWithLocation and building queryURL: check if params == nil or params.ClusterId is empty/zero and return a descriptive error (consistent with other client errors) from the same function that contains the queryURL logic, so the request is not emitted without the required clusterId; update callers/tests if needed to pass a valid ClusterId.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/handlers/v1alpha1/sizer_test.go`:
- Around line 1395-1419: Update the two "returns 404" tests for
GetAssessmentClusterRequirements to also assert the response payload's error
message text: after confirming resp is a
server.GetAssessmentClusterRequirements404JSONResponse, type-assert it, extract
the payload/message field from that response and add an Expect that the message
contains a clear "not found" pattern (for the stored-payload case include the
assessmentID/clusterID context if present, and for the non-existent assessment
case assert it mentions the assessment id or a generic "assessment not found"
phrase) so QE will catch regressions in the not-found messaging.
In `@internal/service/sizer_test.go`:
- Around line 1016-1018: The test currently inspects the mock's internal map
(mockStore.clusterInputs) to assert persistence; instead, call the public
retrieval method GetClusterRequirementsInput with the composed key (assessmentID
and clusterID) and assert it returns a not-found result/error to verify the
service behavior. Locate the assertion that uses mockStore.clusterInputs in the
test for CalculateClusterRequirements (or the test function around it), replace
it with a call to mockStore.GetClusterRequirementsInput(assessmentID, clusterID)
(or the service wrapper used in tests) and assert the returned value indicates
not found (nil/false/error) rather than inspecting mockStore.clusterInputs
directly.
In `@internal/service/sizer.go`:
- Around line 398-400: The trace/span for "calculate_cluster_requirements" is
being marked successful before a fallible DB write; move the success/finish of
that trace and any success logging/metrics to after
s.persistClusterSizingInput(ctx, assessmentID, req) completes successfully, so
that if persistClusterSizingInput returns an error the trace records failure and
the success metric/log is not emitted; update the code paths around
persistClusterSizingInput, the span/trace handling for
calculate_cluster_requirements, and any structured log/metric emission to
reflect error vs success consistently (use the same span variable and set
error/status only on error, then finish the span and emit success logs/metrics
after persistence).
---
Duplicate comments:
In `@internal/api/client/client.gen.go`:
- Around line 1114-1130: The code currently allows params to be nil or
params.ClusterId to be empty and proceeds to build the query; add an early
validation that rejects missing clusterId before calling
runtime.StyleParamWithLocation and building queryURL: check if params == nil or
params.ClusterId is empty/zero and return a descriptive error (consistent with
other client errors) from the same function that contains the queryURL logic, so
the request is not emitted without the required clusterId; update callers/tests
if needed to pass a valid ClusterId.
In `@internal/handlers/v1alpha1/sizer.go`:
- Around line 38-47: The error handling block after GetAssessment is missing a
case for *service.ErrForbidden so forbidden errors are treated as 500; add a
case for *service.ErrForbidden in the switch that logs the error with
logger.Error(err).WithUUID("assessment_id", request.Id).Log() and returns
server.GetAssessmentClusterRequirements403JSONResponse{Message: err.Error()},
nil (mirroring the CalculateAssessmentClusterRequirements handling), leaving the
existing ErrResourceNotFound and default branches 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: ASSERTIVE
Plan: Pro
Run ID: dd582209-e012-47ac-a53b-8596b024dcc5
📒 Files selected for processing (19)
api/v1alpha1/openapi.yamlapi/v1alpha1/spec.gen.goapi/v1alpha1/types.gen.gointernal/api/client/client.gen.gointernal/api/server/server.gen.gointernal/handlers/v1alpha1/mappers/inbound.gointernal/handlers/v1alpha1/mappers/outbound.gointernal/handlers/v1alpha1/sizer.gointernal/handlers/v1alpha1/sizer_test.gointernal/service/errors.gointernal/service/mappers/inbound.gointernal/service/sizer.gointernal/service/sizer_test.gointernal/store/cluster_sizing_input.gointernal/store/cluster_sizing_input_test.gointernal/store/model/cluster_sizing_input.gointernal/store/store.gopkg/migrations/migrations_test.gopkg/migrations/sql/20260326150000_assessment_cluster_sizing_inputs.sql
nirarg
reviewed
Apr 13, 2026
| content: | ||
| application/json: | ||
| schema: | ||
| $ref: "#/components/schemas/ClusterRequirementsStoredInput" |
Collaborator
There was a problem hiding this comment.
Why not just naming it to "ClusterRequirements"?
Collaborator
Author
There was a problem hiding this comment.
the endpoint path is “cluster-requirements”, and we already have the schemas for ClusterRequirementsRequest and ClusterRequirementsResponse, so having a schema as ClusterRequirements may be confusing and not persistent with other schema names.
Collaborator
There was a problem hiding this comment.
Ok, I agree its confusing
But it actually also confusing now that the Get API returnes something else than ClusterRequirementsResponse
I'm not sure about this, but what do you think about having this Get under "/api/v1/assessments/{id}/cluster-requirements/stored-input"
Again, I'm not sure this is the right thing to do
Collaborator
Author
There was a problem hiding this comment.
This makes sense. since the same endpoint path returns different schemas depending on the HTTP method i am going to take your suggestion and change the path accordingly.
Collaborator
Author
There was a problem hiding this comment.
done. GET path: "/api/v1/assessments/{id}/cluster-requirements/stored-input?clusterId=xxx"
| ctx context.Context, | ||
| assessmentID uuid.UUID, | ||
| clusterID string, | ||
| ) (*mappers.ClusterRequirementsInputForm, error) { |
Collaborator
There was a problem hiding this comment.
Its not clear to me why the additional object in service/mappers and not returning the model object
Anyway there is additional conversion layer to the API object
Collaborator
Author
There was a problem hiding this comment.
I’d rather not return the GORM model from the service. The rest of the sizer flow already uses the mapper structs here, and the handler stays responsible for turning that into the OpenAPI type and its consistent with how other services (assesments, sources etc.) use mappers
ronenav
force-pushed
the
4214-sizer-to-db
branch
3 times, most recently
from
cd35aee to
e2a997a
Compare
ronenav
force-pushed
the
4214-sizer-to-db
branch
from
e2a997a to
84ec03c
Compare
nirarg
approved these changes
Apr 16, 2026
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: nirarg The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
ronenav
force-pushed
the
4214-sizer-to-db
branch
from
84ec03c to
3569de4
Compare
|
New changes are detected. LGTM label has been removed. |
…rt and add GET endpoint for PDF reuse Signed-off-by: Ronen Avraham <ravraham@redhat.com>
ronenav
force-pushed
the
4214-sizer-to-db
branch
from
3569de4 to
30009e3
Compare
Collaborator
Author
|
/retest |
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.
Add database persistence for cluster requirements inputs (CPU/memory ratios, node
specs, control plane config)
cluster
/api/v1/assessments/{id}/cluster-requirements?clusterId=Xtoretrieve previously entered inputs
QE tests:
/api/v1/assessments/{id}/cluster-requirements/stored-input?clusterId=XSummary by CodeRabbit
New Features
Behavior changes
Database
Tests