Repository navigation
✨ OCPCLOUD-3513: add a tool to generate manifests-summary - #616
Conversation
|
@hongkailiu: This pull request references OCPCLOUD-3513 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this: Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
added
the
jira/valid-reference
|
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:
WalkthroughAdds a standalone Go CLI tool ( ChangesManifest Summary Generation and Integration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (13 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
/jira refresh |
|
@hongkailiu: This pull request references OCPCLOUD-3513 which is a valid jira issue. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@openshift/tools/generate-manifests-summary/main.go`:
- Line 23: The usage example comment on line 23 references an incorrect flag
name `--output`, but the actual flag defined in the code is
`--manifests-summary-file`. Update the comment example to use the correct flag
name `--manifests-summary-file` instead of `--output` to ensure the
documentation accurately reflects how users should invoke the command.
- Around line 65-217: Add comprehensive unit tests for the new parsing and
update functions introduced in this change. Create tests for parseAPIVersion to
verify it correctly splits group and version components including edge cases
with empty strings and various formats. Add tests for parseDocument to validate
it handles missing kind and metadata.name fields with appropriate error
messages. Add tests for parseAllDocuments to verify it correctly decodes
multiple YAML documents and properly reports decode errors with document
indices. Add tests for readExistingSummary to handle missing output files and
invalid YAML content. Add tests for updateSummary to verify that manifests are
sorted deterministically by the correct field order (kind, group, version,
namespace, name) and that the profile is correctly updated in the summary. Add
tests for writeOutput to verify it marshals and writes files correctly. These
tests should cover both success and failure paths to ensure robust error
handling.
- Around line 131-138: The code currently treats missing or empty summary files
as fatal errors in the readExistingSummary function call around line 132, which
crashes on first-run generation when no summary file exists yet. Instead of
calling klog.Fatalf, modify the error handling to gracefully return an empty or
default summary when the file does not exist. Additionally, refactor the run
function to return errors instead of calling klog.Fatalf directly around line
134, allowing the caller to handle errors appropriately. Finally, fix the nil
map assignment issue around lines 185-186 by initializing the map or checking
for nil before attempting to assign values to it.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 69187914-8b43-496b-9f36-0c44af02a96d
📒 Files selected for processing (4)
openshift/Makefileopenshift/manifests-summary.yamlopenshift/tools/generate-manifests-summary/main.goopenshift/tools/go.mod
c9a72a0 to
5710224
Compare
hongkailiu
changed the title
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
93e1a7e to
f2e3761
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
openshift/tools/generate-manifests-summary/main.go (2)
94-94:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHandle
Close()error instead of ignoring it.
defer file.Close()drops the returned error, which violates the Go security guideline and can hide I/O issues.As per coding guidelines, “Never ignore error returns.”
Proposed fix
-func run(inputPath, outputPath, profileName string) error { +func run(inputPath, outputPath, profileName string) (err error) { klog.V(1).InfoS("Opening input file", "path", inputPath) - file, err := os.Open(inputPath) + file, err := os.Open(inputPath) if err != nil { return fmt.Errorf("failed to open input file %q: %w", inputPath, err) } - defer file.Close() + defer func() { + if cerr := file.Close(); cerr != nil && err == nil { + err = fmt.Errorf("failed to close input file %q: %w", inputPath, cerr) + } + }()🤖 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 `@openshift/tools/generate-manifests-summary/main.go` at line 94, The defer statement calling file.Close() is ignoring the error returned by the Close() method, which violates Go security guidelines. To fix this, modify the defer block to properly handle the error returned by Close(). This typically involves using a named return variable in the function signature and updating it within the defer block to capture any close errors, ensuring the error is logged or propagated rather than silently dropped.Source: Coding guidelines
66-83:⚠️ Potential issue | 🟠 MajorNever ignore file close errors; handle empty documents defensively.
The manifest file has no empty YAML docs in practice, so the empty-document issue is defensive rather than critical. However, there is a real Go security issue: line 94 uses
defer file.Close()without checking the returned error, which violates the coding guideline "Never ignore error returns" for Go files.Wrap the
Close()call to capture and handle any errors:func run(inputPath, outputPath, profileName string) error { klog.V(1).InfoS("Opening input file", "path", inputPath) file, err := os.Open(inputPath) if err != nil { return fmt.Errorf("failed to open input file %q: %w", inputPath, err) } - defer file.Close() + defer func() { + if err := file.Close(); err != nil { + klog.V(1).InfoS("Warning: failed to close file", "path", inputPath, "err", err) + } + }()Regarding empty documents: the empty-doc handling proposed in the original review is still defensible as a robustness improvement, even though the current manifests contain none.
🤖 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 `@openshift/tools/generate-manifests-summary/main.go` around lines 66 - 83, Locate the defer file.Close() statement in the function and wrap it to capture and handle the error return value from Close(). Instead of using defer with an unchecked Close(), use a proper error handling mechanism to check if Close() returns an error, and either log it or return it as part of the function's error handling flow. This ensures compliance with Go's error handling guidelines that require all error returns to be explicitly checked.
🤖 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.
Outside diff comments:
In `@openshift/tools/generate-manifests-summary/main.go`:
- Line 94: The defer statement calling file.Close() is ignoring the error
returned by the Close() method, which violates Go security guidelines. To fix
this, modify the defer block to properly handle the error returned by Close().
This typically involves using a named return variable in the function signature
and updating it within the defer block to capture any close errors, ensuring the
error is logged or propagated rather than silently dropped.
- Around line 66-83: Locate the defer file.Close() statement in the function and
wrap it to capture and handle the error return value from Close(). Instead of
using defer with an unchecked Close(), use a proper error handling mechanism to
check if Close() returns an error, and either log it or return it as part of the
function's error handling flow. This ensures compliance with Go's error handling
guidelines that require all error returns to be explicitly checked.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8ca13b77-23b7-4831-83d3-5cbe0142ffb8
📒 Files selected for processing (2)
openshift/manifests-summary.yamlopenshift/tools/generate-manifests-summary/main.go
✅ Files skipped from review due to trivial changes (1)
- openshift/manifests-summary.yaml
f2e3761 to
af4c6b5
Compare
4ddbb25 to
b19a734
Compare
hongkailiu
changed the title
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
| default: | ||
| - apiVersion: rbac.authorization.k8s.io/v1 | ||
| kind: ClusterRole | ||
| name: capa-manager-role | ||
| - apiVersion: rbac.authorization.k8s.io/v1 | ||
| kind: ClusterRoleBinding | ||
| name: capa-manager-rolebinding | ||
| - apiVersion: apiextensions.k8s.io/v1 | ||
| kind: CustomResourceDefinition | ||
| name: awsclustercontrolleridentities.infrastructure.cluster.x-k8s.io | ||
| - apiVersion: apiextensions.k8s.io/v1 | ||
| kind: CustomResourceDefinition | ||
| name: awsclusters.infrastructure.cluster.x-k8s.io | ||
| - apiVersion: apiextensions.k8s.io/v1 | ||
| kind: CustomResourceDefinition | ||
| name: awsmachines.infrastructure.cluster.x-k8s.io | ||
| - apiVersion: apiextensions.k8s.io/v1 | ||
| kind: CustomResourceDefinition | ||
| name: awsmachinetemplates.infrastructure.cluster.x-k8s.io | ||
| - apiVersion: apps/v1 | ||
| kind: Deployment | ||
| name: capa-controller-manager |
There was a problem hiding this comment.
I like the output!
But I wonder if we can avoid defining a new go module to do this.
Could we leverage existing tools like yq/jq or similar to generate this? Thanks!
There was a problem hiding this comment.
I got the impression that GoLang is alright too.
Moreover, the tools for GoLang are already used in Makefile
cluster-api-provider-aws/openshift/Makefile
Lines 16 to 18 in 90f95c2
go get vs go run: what is the difference?
Just to a confirmation before switching.
You want to implement the same function with Shell + yq/jq, correct?
Let me try it and get back to you.
There was a problem hiding this comment.
It has go run too.
There was a problem hiding this comment.
Switched to "Shell + yq/jq" implementation.
There was a problem hiding this comment.
yq seems not available.
If we want to go to that direction, we might need to install them here.
jq is easy while yq might be non-trivial.
There was a problem hiding this comment.
There was a problem hiding this comment.
Do you think this could be something that manifests-gen can spit out at the end if the --output-resource-summary or something similar is provided? I'm not against doing this in go, is just that I'm trying to be conscious of extra added complexity, especially when we were defining a new binary in the previous iteration. WDYT?
Having it in manifests-gen would solve the centralization problem too: https://github.com/openshift/cluster-api-provider-aws/pull/616/changes#r3736016878
There was a problem hiding this comment.
9a976d1 to
dfa5b4a
Compare
hongkailiu
changed the title
|
I am wondering why /test verify-commits |
|
/test verify |
| .PHONY: vendor-tools | ||
| vendor-tools: | ||
| cd tools && go mod tidy && go mod vendor | ||
| # recover the transformers folder which is removed by `go mod vendor` above | ||
| git checkout "$(MAKEFILE_DIR)/tools/vendor/github.com/openshift/cluster-capi-operator/manifests-gen/transformers" |
There was a problem hiding this comment.
Is this now needed because we use MAKEFILE_DIR ?
Where is it invoked?
There was a problem hiding this comment.
How do we bump the following dep?
In my mind, we will do cd tools && go mod tidy && go mod vendor after bumping for the sake of vendor's sanity. Usually there is a presubmit enforcing it but not for this repo apparently. Otherwise, it would caught #618 as it changes the vendor without modifying go.mod.
Even if we do it without this pull, https://github.com/openshift/cluster-capi-operator/tree/main/manifests-gen/transformers will be removed. Gemini tells me it is because go mod vendor does not pull a folder which has no GoLang files, such as the transformers folder.
This target is not used in CI. But I feel it is useful for a dev to bump the version github.com/openshift/cluster-capi-operator/manifests-gen.
But I can remove it if you do not like it.
There was a problem hiding this comment.
How do we bump the following dep?
I found the update-manifests-gen target.
Moved git checkout under it.
The reason that we need git checkout stays the same as described above.
d98cfda to
bfd148e
Compare
| .PHONY: update-manifests-gen | ||
| update-manifests-gen: | ||
| cd tools && go get github.com/openshift/cluster-capi-operator/manifests-gen && go mod tidy && go mod vendor | ||
| git checkout "tools/vendor/github.com/openshift/cluster-capi-operator/manifests-gen/transformers" |
There was a problem hiding this comment.
I'm not following this part, is this required by this PR
There was a problem hiding this comment.
My answer is here.
I am fine if you want me to remove it.
There was a problem hiding this comment.
Removed in the last commit.
b43289c to
68882e1
Compare
|
/test unit |
|
Scheduling tests matching the |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: damdo 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 |
openshift-ci
Bot
added
the
approved
|
/hold The openshift/Makefile change needs to be in an |
openshift-ci
Bot
added
the
do-not-merge/hold
Signed-off-by: Damiano Donati <damiano.donati@gmail.com>
Signed-off-by: Damiano Donati <damiano.donati@gmail.com>
Signed-off-by: Damiano Donati <damiano.donati@gmail.com>
68882e1 to
e727b9b
Compare
|
/lgtm |
|
Scheduling tests matching the |
|
@hongkailiu: 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. |
|
/unhold /verified by @damdo the generated manifests look correct to me |
openshift-ci
Bot
removed
the
do-not-merge/hold
openshift-ci-robot
added
the
verified
|
@damdo: This PR has been marked as verified by DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
3 participants
The new target
manifests-summarygenerates the summary of capi-operator-manifests/default/manifests.yaml.Requires #618 to go first.