Integrate models-schema into codegen and fix openapi verify issues#2904
Integrate models-schema into codegen and fix openapi verify issues#2904JoelSpeed wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
Hello @JoelSpeed! Some important instructions when contributing to openshift/api: |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe PR updates the OpenAPI generation workflow to use integrated code generation, changes the default output package path, and stops cleaning 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)Error: build linters: unable to load custom analyzer "kubeapilinter": tools/_output/bin/kube-api-linter.so, plugin: not implemented Comment |
openshift-ci
Bot
added
the
size/XL
|
[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. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tools/codegen/pkg/openapi/openapi.go (1)
29-46: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKeep verify generation isolated from the checked-in output.
goOutputPackagePathandoutputFileare computed before line 42 swapsoutputPackagePathto the temp dir, so verify mode still writes into the realopenapi/generated_openapitree. Then line 83 compares that same path as both generated and current output, allowing Go verification to pass after mutating the workspace.Proposed direction
originalOutputPackagePath := outputPackagePath - goOutputPackagePath := filepath.Join(outputPackagePath, generatedOpenAPI) - outputFile := filepath.Join(goOutputPackagePath, outputFileName) + currentGoOutputPackagePath := filepath.Join(originalOutputPackagePath, generatedOpenAPI) + currentOutputFile := filepath.Join(currentGoOutputPackagePath, outputFileName) + goOutputPackagePath := currentGoOutputPackagePath if verify { outputPackageBase := filepath.Base(outputPackagePath) @@ outputPackagePath = filepath.Join(tmpDir, outputPackageBase) + goOutputPackagePath = filepath.Join(outputPackagePath, generatedOpenAPI) } arguments := args.New() arguments.OutputDir = goOutputPackagePath - arguments.OutputPkg = goOutputPackagePath + arguments.OutputPkg = currentGoOutputPackagePath @@ - if err := verifyGoFile(outputFile, goOutputPackagePath, outputFileName); err != nil { + if err := verifyGoFile(currentOutputFile, goOutputPackagePath, outputFileName); err != nil { return fmt.Errorf("error verifying generated openapi Go file: %w", err) } @@ - return verifyJSONSchema(goOutputPackagePath, originalOutputPackagePath) + return verifyJSONSchema(currentGoOutputPackagePath, originalOutputPackagePath)Also applies to: 83-88
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/codegen/pkg/openapi/openapi.go` around lines 29 - 46, Verify mode is still using the checked-in output path because `goOutputPackagePath` and `outputFile` are computed before `verify` swaps `outputPackagePath` to the temp directory. Recompute the Go output path and output file after the temp-path reassignment in the openapi generation flow, and make the later compare step use the temp-generated path versus the real checked-in path so `openapi/generated_openapi` is never mutated during verification. Update the logic around the openapi generation function and the compare section that reads the generated output to ensure verify mode stays isolated.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@hack/update-openapi.sh`:
- Line 10: The script invocation in the update-openapi flow leaves the
${SCRIPT_ROOT} path expansion unquoted, which can break when the checkout path
contains spaces. Update the call in hack/update-openapi.sh so the
hack/update-codegen.sh path is quoted while keeping the existing
GENERATOR/openapi and EXTRA_ARGS behavior intact.
In `@tools/codegen/pkg/openapi/openapi.go`:
- Around line 244-252: Both `tidyCmd` and the later `cmd` in the OpenAPI codegen
flow are created with `exec.Command`, so they can hang indefinitely during
module resolution. Update the code that runs `go mod tidy` and `go run .` to use
`exec.CommandContext` with a timeout-backed `context.Context`, and propagate
that context through the surrounding helper/function that contains these
commands so cancellation works cleanly. Also add any needed context-related
import and keep the existing error handling around the `tidyCmd.Run()` and
`cmd.Run()` paths.
---
Outside diff comments:
In `@tools/codegen/pkg/openapi/openapi.go`:
- Around line 29-46: Verify mode is still using the checked-in output path
because `goOutputPackagePath` and `outputFile` are computed before `verify`
swaps `outputPackagePath` to the temp directory. Recompute the Go output path
and output file after the temp-path reassignment in the openapi generation flow,
and make the later compare step use the temp-generated path versus the real
checked-in path so `openapi/generated_openapi` is never mutated during
verification. Update the logic around the openapi generation function and the
compare section that reads the generated output to ensure verify mode stays
isolated.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 70d87c72-2f14-497a-9668-510dae24de74
⛔ Files ignored due to path filters (2)
openapi/cmd/models-schema/main.gois excluded by!openapi/**openapi/openapi.jsonis excluded by!openapi/**
📒 Files selected for processing (4)
Makefilehack/update-openapi.shtools/codegen/pkg/openapi/generator.gotools/codegen/pkg/openapi/openapi.go
|
|
||
| ./models-schema | jq '.' > ${output_package}/openapi.json | ||
| # Generate both Go and JSON OpenAPI schemas using the integrated codegen tool | ||
| GENERATOR=openapi EXTRA_ARGS=--openapi:output-package-path=${output_path} ${SCRIPT_ROOT}/hack/update-codegen.sh |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Quote the script path invocation.
Line 10 leaves ${SCRIPT_ROOT} unquoted when invoking hack/update-codegen.sh, so checkouts under paths with spaces can fail before codegen starts.
Proposed fix
-GENERATOR=openapi EXTRA_ARGS=--openapi:output-package-path=${output_path} ${SCRIPT_ROOT}/hack/update-codegen.sh
+GENERATOR=openapi EXTRA_ARGS=--openapi:output-package-path=${output_path} "${SCRIPT_ROOT}/hack/update-codegen.sh"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| GENERATOR=openapi EXTRA_ARGS=--openapi:output-package-path=${output_path} ${SCRIPT_ROOT}/hack/update-codegen.sh | |
| GENERATOR=openapi EXTRA_ARGS=--openapi:output-package-path=${output_path} "${SCRIPT_ROOT}/hack/update-codegen.sh" |
🧰 Tools
🪛 Shellcheck (0.11.0)
[info] 10-10: Double quote to prevent globbing and word splitting.
(SC2086)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@hack/update-openapi.sh` at line 10, The script invocation in the
update-openapi flow leaves the ${SCRIPT_ROOT} path expansion unquoted, which can
break when the checkout path contains spaces. Update the call in
hack/update-openapi.sh so the hack/update-codegen.sh path is quoted while
keeping the existing GENERATOR/openapi and EXTRA_ARGS behavior intact.
Source: Linters/SAST tools
There was a problem hiding this comment.
While unlikely to be a major issue, this seems like a reasonable change to make while we are here.
I wouldn't block on this though.
| // Run go mod tidy to fix dependencies | ||
| tidyCmd := exec.Command("go", "mod", "tidy") | ||
| tidyCmd.Dir = tempDir | ||
| if err := tidyCmd.Run(); err != nil { | ||
| return nil, fmt.Errorf("failed to run go mod tidy: %w", err) | ||
| } | ||
|
|
||
| // Run the temporary program and capture its output | ||
| cmd := exec.Command("go", "run", ".") |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Bound external go commands with a context.
go mod tidy and go run . can block on module/toolchain resolution, which can hang verification indefinitely in CI. Use exec.CommandContext with a timeout. As per path instructions, **/*.go: “context.Context for cancellation and timeouts”.
Proposed fix
+ ctx, cancel := context.WithTimeout(context.Background(), 5*time.Minute)
+ defer cancel()
+
// Run go mod tidy to fix dependencies
- tidyCmd := exec.Command("go", "mod", "tidy")
+ tidyCmd := exec.CommandContext(ctx, "go", "mod", "tidy")
tidyCmd.Dir = tempDir
if err := tidyCmd.Run(); err != nil {
return nil, fmt.Errorf("failed to run go mod tidy: %w", err)
}
+ runCtx, runCancel := context.WithTimeout(context.Background(), 5*time.Minute)
+ defer runCancel()
+
// Run the temporary program and capture its output
- cmd := exec.Command("go", "run", ".")
+ cmd := exec.CommandContext(runCtx, "go", "run", ".")Also add the imports:
+ "context"
"os/exec"
+ "time"Also applies to: 260-262
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/codegen/pkg/openapi/openapi.go` around lines 244 - 252, Both `tidyCmd`
and the later `cmd` in the OpenAPI codegen flow are created with `exec.Command`,
so they can hang indefinitely during module resolution. Update the code that
runs `go mod tidy` and `go run .` to use `exec.CommandContext` with a
timeout-backed `context.Context`, and propagate that context through the
surrounding helper/function that contains these commands so cancellation works
cleanly. Also add any needed context-related import and keep the existing error
handling around the `tidyCmd.Run()` and `cmd.Run()` paths.
Source: Path instructions
JoelSpeed
force-pushed
the
integrated-models-schema
branch
from
c184ba0 to
7bc1397
Compare
|
@JoelSpeed: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
|
||
| ./models-schema | jq '.' > ${output_package}/openapi.json | ||
| # Generate both Go and JSON OpenAPI schemas using the integrated codegen tool | ||
| GENERATOR=openapi EXTRA_ARGS=--openapi:output-package-path=${output_path} ${SCRIPT_ROOT}/hack/update-codegen.sh |
There was a problem hiding this comment.
While unlikely to be a major issue, this seems like a reasonable change to make while we are here.
I wouldn't block on this though.
| packageName := filepath.Base(schemaSourcePackage) | ||
|
|
||
| // Create temporary main.go that calls the generated function | ||
| tempMainContent := fmt.Sprintf(`package main |
There was a problem hiding this comment.
Why are we needing to produce this temporary go program to run?
2 participants
Models schema has historically been a separate step, we often forget about it and this has lead to incomplete verification of the openapi.json schema over time.
This PR integrates the model schema stage directly into
codegen, including integrating the verify step. The existing file locations and output for both the Go models and json schema remain as they were.