feat: add Kubernetes-layer dry-run validation for WorkspaceKind … #639
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
bhaktinarvekar
changed the title
…creation This update enhances the WorkspaceKind POST API to properly perform dry-run validation at the Kubernetes API layer. When `dry_run=true` is specified, the request now invokes controller-runtime’s Create API with dry-run semantics, allowing validation errors to be surfaced directly from Kubernetes (e.g., schema or webhook errors). Changes include: - Added CreateOptions with DryRunAll for dry-run requests - Unified create logic for both dry-run and normal requests - Improved error propagation and response mapping - Updated tests for dry-run and standard create flows Signed-off-by: Bhakti Narvekar <[email protected]>
bhaktinarvekar
force-pushed
the
issue_417_v1
branch
from
485824d to
61d200f
Compare
|
/ok-to-test |
There was a problem hiding this comment.
Thanks for your continued diligence on this PR - we are very close 💯
In addition to addressing the requested changes below (reach out to me if you have questions/concerns) - I noticed the make test invocation also results in errors...
While I did add the ok-to-test label to see this failures in GitHub - in future please me mindful of running make test locally before submitting a PR to ensure the tests you are adding are in fact passing before marking the PR ready.
🙇
| if !a.ValidateContentType(w, r, MediaTypeYaml) { | ||
| if hasDryRun { | ||
| // Special case: dry_run provided with wrong media type → 400 | ||
| a.badRequestResponse(w, r, | ||
| fmt.Errorf("dry_run is only supported when Content-Type: %s", MediaTypeYaml)) | ||
| } else { | ||
| // Normal case: wrong media type → 415 | ||
| a.unsupportedMediaTypeResponse(w, r, | ||
| fmt.Errorf("only %s is supported", MediaTypeYaml)) | ||
| } |
There was a problem hiding this comment.
I realize this implementation is specifically following the GH issue guidance... but now that I am re-reviewing things (and note: i wrote that issue!) - I can't for the life of me think why this would be desirable!
We shouldn't carve out a specific error message here for dry_run parameter... if the ValidateContentType check fails... we should just be doing
| if !a.ValidateContentType(w, r, MediaTypeYaml) { | |
| if hasDryRun { | |
| // Special case: dry_run provided with wrong media type → 400 | |
| a.badRequestResponse(w, r, | |
| fmt.Errorf("dry_run is only supported when Content-Type: %s", MediaTypeYaml)) | |
| } else { | |
| // Normal case: wrong media type → 415 | |
| a.unsupportedMediaTypeResponse(w, r, | |
| fmt.Errorf("only %s is supported", MediaTypeYaml)) | |
| } | |
| if !a.ValidateContentType(w, r, MediaTypeYaml) { | |
| a.unsupportedMediaTypeResponse(w, r, | |
| fmt.Errorf("only %s is supported", MediaTypeYaml)) |
This also means we don't need this additional "look up":
hasDryRun := r.URL.Query().Get("dry_run") != ""
which is a nice added benefit - as the URL.Query.Get(...) code now can purely be contained within the helper function.
sorry for that poor initial guidance!
| // Return appropriate response based on dry-run | ||
| if isDryRun { | ||
| responseEnvelope := &WorkspaceKindEnvelope{Data: *createdWorkspaceKind} | ||
| a.dataResponse(w, r, responseEnvelope) | ||
| return | ||
| } | ||
|
|
There was a problem hiding this comment.
I think we can do a little better job here combining L239-243 with L246-249
in both cases - the following invocation is the same:
responseEnvelope := &WorkspaceKindCreateEnvelope{Data: createdWorkspaceKind}
- I notice you are passing a pointer in your newly added code (which is preferred style now!)
But i think refactoring this into an if/else (pullin gout the responseEnvelope declaration) might be a little clearer expressing intent
2 participants
…creation
This update enhances the WorkspaceKind POST API to properly perform dry-run validation at the Kubernetes API layer. When
dry_run=trueis specified, the request now invokes controller-runtime’s Create API with dry-run semantics, allowing validation errors to be surfaced directly from Kubernetes (e.g., schema or webhook errors).Changes include: