Make dry-run also generate cluster-resources#1394
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
WalkthroughDryRun now writes mapping output immediately, emits missing-image files via a helper, and conditionally performs best-effort cluster resource generation (IDMS/ITMS, CatalogSource, ClusterCatalog, signature ConfigMap, and UpdateService for OCP releases); tests are refactored with shared setup and a new cluster-resources test. ChangesCluster Resource Generation & DryRun flow
sequenceDiagram
participant User
participant DryRun
participant ProcessImages
participant ClusterResources
participant Generators
participant FS as FileSystem
User->>DryRun: invoke DryRun()
DryRun->>ProcessImages: process images
ProcessImages-->>DryRun: images list, nbMissingImgs
DryRun->>FS: write mapping.txt
DryRun->>DryRun: writeMissingImagesFile(nbMissingImgs)
DryRun->>ClusterResources: generateClusterResources(allImages) (if not to-disk)
ClusterResources->>Generators: run IDMS/ITMS, Catalog, SigConfigMap generators
Generators->>FS: write manifests/catalogs
alt OCP release present
ClusterResources->>Generators: resolve graph/release, generate UpdateService
Generators->>FS: write UpdateService manifest
end
DryRun-->>User: dry-run complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 10 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (10 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 |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: javipolo 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: 3
🧹 Nitpick comments (2)
internal/pkg/cli/dryrun.go (1)
77-91: Consider downgrading cluster-resource failures to warnings, and drop theClusterResources == nilguard in favor of test mocks.Two small points:
DryRunpreviously always succeeded when the mapping and (optionally) missing files were produced. Now any failure fromIDMS_ITMSGenerator,CatalogSourceGenerator,ClusterCatalogGenerator, orUpdateServiceGeneratorfails the whole dry-run.GenerateSignatureConfigMapis already correctly downgraded to a warning — consider the same treatment for the others so cluster-resource generation issues don't mask the main dry-run output.- The
if o.ClusterResources == nilcheck (with the comment "e.g., in tests") is production code working around test setup. The existingexecutor_test.gohas aMockClusterResourcesno-op implementation — updating the olderTestDryRunsub-tests to inject that mock (like the new sub-test does withclusterresources.New) removes the need for the nil guard.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/pkg/cli/dryrun.go` around lines 77 - 91, Adjust generateClusterResourcesForDryRun so cluster-resource generator failures are logged as warnings instead of returning errors: for IDMS_ITMSGenerator, CatalogSourceGenerator, ClusterCatalogGenerator, and UpdateServiceGenerator catch errors from their calls, call o.Log.Warnf with a descriptive message and continue (mirroring the existing treatment of GenerateSignatureConfigMap), and return nil at the end unless a non-cluster-resource fatal condition occurs; also remove the if o.ClusterResources == nil guard and update tests to inject MockClusterResources (e.g., in TestDryRun) so production code no longer compensates for test setup.internal/pkg/cli/dryrun_test.go (1)
199-203: Sharedglobalis mutated across sub-tests.
globalis declared once at the top ofTestDryRunand each sub-test doesglobal.WorkingDir = testFolder. That's fine whilet.Runsub-tests execute serially, but it's a foot-gun if anyone later addst.Parallel(). Consider constructing a fresh*mirror.GlobalOptionsper sub-test.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/pkg/cli/dryrun_test.go` around lines 199 - 203, The test mutates the package-level variable global across sub-tests (global.WorkingDir = testFolder), which is unsafe for parallel runs; instead, create a fresh *mirror.GlobalOptions per sub-test (e.g., local := &mirror.GlobalOptions{WorkingDir: testFolder, ...}) inside the t.Run closure and use that local instance in calls that currently rely on the package-global, or assign the package-global once from that local before invoking the code under test. Update uses of global in TestDryRun sub-tests to reference the new local variable (or set global = local at the start of the subtest) so each sub-test has its own independent GlobalOptions.
🤖 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/pkg/cli/dryrun_test.go`:
- Around line 282-289: The test's assertions are too weak — they only check that
cluster-resources/ exists but not that the new manifests are created or correct;
update the test in dryrun_test.go to assert specific files exist under
clusterResourcesDir (e.g., idms-oc-mirror.yaml, itms-oc-mirror.yaml, the catalog
source and cluster catalog manifests, the signature ConfigMap, and the
UpdateService manifest for the OCPRelease image) and then read the UpdateService
manifest file to assert its graph image reference matches the value produced by
constructGraphImageRef (or at least contains the expected repository/name like
"graph-data" and not the hardcoded "localhost:5000"), so the test fails if the
OCP-release path or graph image generation regresses.
In `@internal/pkg/cli/dryrun.go`:
- Around line 142-169: Replace the custom, hardcoded logic in
constructGraphImageRef with a call to the real implementation: invoke
o.Release.GraphImage() and return its result (propagate any error), removing the
unused releaseImageParts split and the bogus localhost:5000 fallback and the
incorrect "openshift/graph-data:latest" name; this ensures the dry-run uses the
same registry and image name (openshift/graph-image) as the real flow and stops
relying on ImageSetConfigurationSpec.Mirror.AdditionalImages parsing.
- Around line 116-137: The test data includes release image Origins with the
"docker://" transport prefix which must be stripped before calling
UpdateServiceGenerator; in the loop that finds v2alpha1.TypeOCPRelease (using
allImages and releaseImage), call strings.TrimPrefix(img.Origin,
consts.DockerProtocol) (or trim releaseImage right after assignment) and pass
that trimmed value to o.ClusterResources.UpdateServiceGenerator(graphImageRef,
releaseImage) (ensure constructGraphImageRef still receives the correct input),
so UpdateServiceGenerator gets the unprefixed release image string matching
production ReleaseImage() behavior.
---
Nitpick comments:
In `@internal/pkg/cli/dryrun_test.go`:
- Around line 199-203: The test mutates the package-level variable global across
sub-tests (global.WorkingDir = testFolder), which is unsafe for parallel runs;
instead, create a fresh *mirror.GlobalOptions per sub-test (e.g., local :=
&mirror.GlobalOptions{WorkingDir: testFolder, ...}) inside the t.Run closure and
use that local instance in calls that currently rely on the package-global, or
assign the package-global once from that local before invoking the code under
test. Update uses of global in TestDryRun sub-tests to reference the new local
variable (or set global = local at the start of the subtest) so each sub-test
has its own independent GlobalOptions.
In `@internal/pkg/cli/dryrun.go`:
- Around line 77-91: Adjust generateClusterResourcesForDryRun so
cluster-resource generator failures are logged as warnings instead of returning
errors: for IDMS_ITMSGenerator, CatalogSourceGenerator, ClusterCatalogGenerator,
and UpdateServiceGenerator catch errors from their calls, call o.Log.Warnf with
a descriptive message and continue (mirroring the existing treatment of
GenerateSignatureConfigMap), and return nil at the end unless a
non-cluster-resource fatal condition occurs; also remove the if
o.ClusterResources == nil guard and update tests to inject MockClusterResources
(e.g., in TestDryRun) so production code no longer compensates for test setup.
🪄 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: e0215fdc-bd4e-4be5-9a4a-e764c5506b54
📒 Files selected for processing (2)
internal/pkg/cli/dryrun.gointernal/pkg/cli/dryrun_test.go
|
@javipolo could you please have a look on the codeRabbit suggestions and also the linter job that is failing? |
javipolo
force-pushed
the
dry-run-cluster-resources
branch
from
73fa5c4 to
550eb67
Compare
Signed-off-by: Javi Polo <jpolo@redhat.com>
javipolo
force-pushed
the
dry-run-cluster-resources
branch
from
550eb67 to
629bba3
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/pkg/cli/dryrun.go (1)
63-69:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard
o.ClusterResourcesbefore generator calls.
generateClusterResourcesdereferenceso.ClusterResourcesunconditionally. In non-M2D dry-run, an uninitialized generator will panic instead of returning a controlled error.Proposed fix
func (o *ExecutorSchema) generateClusterResources(ctx context.Context, allImages []v2alpha1.CopyImageSchema) error { + if o.ClusterResources == nil { + return fmt.Errorf("cluster resources generator is not initialized") + } o.Log.Info(emoji.PageFacingUp + " Generating cluster resources...")🤖 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 `@internal/pkg/cli/dryrun.go` around lines 63 - 69, generateClusterResources currently dereferences o.ClusterResources without checking for nil which can panic; add a guard at the start of generateClusterResources that checks if o.ClusterResources is nil and return a controlled error (or wrap with context) before calling any generator methods such as IDMS_ITMSGenerator (and any other generator methods on ClusterResources) so the function fails gracefully when the generator is uninitialized.
🤖 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 `@internal/pkg/cli/dryrun_test.go`:
- Around line 147-153: The test uses t.Errorf for unrecoverable setup failures
(the storage cache init and the registry.NewRegistry call), which allows the
test to continue with invalid state; replace those t.Errorf calls with t.Fatalf
so the test aborts immediately on these setup errors—specifically update the
error checks around the storage cache initialization and the
registry.NewRegistry(...) invocation to call t.Fatalf with the error message
instead of t.Errorf.
---
Outside diff comments:
In `@internal/pkg/cli/dryrun.go`:
- Around line 63-69: generateClusterResources currently dereferences
o.ClusterResources without checking for nil which can panic; add a guard at the
start of generateClusterResources that checks if o.ClusterResources is nil and
return a controlled error (or wrap with context) before calling any generator
methods such as IDMS_ITMSGenerator (and any other generator methods on
ClusterResources) so the function fails gracefully when the generator is
uninitialized.
🪄 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: b8cbcf9c-1db0-4e55-8c3f-3de6b7951298
📒 Files selected for processing (2)
internal/pkg/cli/dryrun.gointernal/pkg/cli/dryrun_test.go
javipolo
force-pushed
the
dry-run-cluster-resources
branch
2 times, most recently
from
87694e9 to
c64b6bf
Compare
Signed-off-by: Javi Polo <jpolo@redhat.com>
javipolo
force-pushed
the
dry-run-cluster-resources
branch
from
c64b6bf to
ed32fdf
Compare
|
@javipolo: 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. |
aguidirh
left a comment
aguidirh
left a comment
There was a problem hiding this comment.
Thanks for the changes, it is in a much better shape now!
I've added few comments.
| if len(imagesAvailable) > 0 { | ||
| o.Log.Info("all %d images required for mirroring are available in local cache. You may proceed with mirroring from disk to disconnected registry", len(imagesAvailable)) | ||
| } |
There was a problem hiding this comment.
This is never going to be true, so we can safely remove it. I know it does not come from this PR, but since we are changing the code we can remove it.
There was a problem hiding this comment.
I think the imagesAvailable variable is initialized but never updated, so maybe we can remove it altogether
| nbMissingImgs := 0 | ||
| var buff bytes.Buffer | ||
| var missingImgsBuff bytes.Buffer | ||
| nbMissingImgs = o.processImages(ctx, allImages, &buff, &missingImgsBuff) |
There was a problem hiding this comment.
nit suggestion:
| nbMissingImgs := 0 | |
| var buff bytes.Buffer | |
| var missingImgsBuff bytes.Buffer | |
| nbMissingImgs = o.processImages(ctx, allImages, &buff, &missingImgsBuff) | |
| var buff bytes.Buffer | |
| var missingImgsBuff bytes.Buffer | |
| nbMissingImgs := o.processImages(ctx, allImages, &buff, &missingImgsBuff) |
| DestImage: destOpts, | ||
| RetryOpts: retryOpts, | ||
| IsDryRun: true, | ||
| Mode: mirror.MirrorToMirror, |
There was a problem hiding this comment.
Can we add a scenario to check if m2d is not going to generate cluster resources? It could be using a table driven test.
| for _, img := range imgs { | ||
| assert.Contains(t, mapping, img.Source+"="+img.Destination) | ||
| // Verify UpdateService manifest exists for OCP release images | ||
| updateServiceFiles, err := filepath.Glob(filepath.Join(clusterResourcesPath, "*updateservice*.yaml")) |
There was a problem hiding this comment.
| updateServiceFiles, err := filepath.Glob(filepath.Join(clusterResourcesPath, "*updateservice*.yaml")) | |
| updateServiceFiles, err := filepath.Glob(filepath.Join(clusterResourcesPath, "*updateservice*.yaml")) | |
| require.NotEmpty(t, updateServiceFiles, "UpdateService manifest should be generated when platform.graph=true") | |
adolfo-ab
left a comment
adolfo-ab
left a comment
There was a problem hiding this comment.
Thanks for the PR @javipolo !
The only thing that concerns me is that now we have some code duplication for the creation of cluster resources, which is also done in executor.go for m2m and d2m. Maybe we can encapsulate the code to generate the cluster resources into a function (your PR is already doing this part) and reuse them in the three places.
Maybe we can do this in a follow-up PR, for the sake of not blocking this PR much longer. Wdyt @aguidirh @r4f4 @dorzel ?
| if len(imagesAvailable) > 0 { | ||
| o.Log.Info("all %d images required for mirroring are available in local cache. You may proceed with mirroring from disk to disconnected registry", len(imagesAvailable)) | ||
| } |
There was a problem hiding this comment.
I think the imagesAvailable variable is initialized but never updated, so maybe we can remove it altogether
| } | ||
|
|
||
| missingImgsPath := filepath.Join(testFolder, dryRunOutDir, missingImgsFile) | ||
| assert.FileExists(t, mappingPath) |
There was a problem hiding this comment.
| assert.FileExists(t, mappingPath) | |
| assert.FileExists(t, missingImgsPath) |
3 participants
Description
Create cluster-resource manifests while running with
--dry-runflagGithub / Jira issue:
https://redhat.atlassian.net/browse/RFE-4107
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Run
oc-mirrorwith--dry-runand manifests should apear in working-dir/cluster-resourcesIt was tested manually
Expected Outcome
Manifests should be present in
cluster-resourcesdirectorySummary by CodeRabbit
New Features
Tests