OCPBUGS-62875: Pass dry-run option to all create API calls #2117
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
openshift-ci-robot
added
jira/severity-moderate
|
@liouk: This pull request references Jira Issue OCPBUGS-62875, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: The bug has been updated to refer to the pull request using the external bug tracker. In 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. |
WalkthroughThis PR systematically replaces hardcoded empty Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes The review scope is broad (12 files affected), but the changes are highly homogeneous—the same refactoring pattern is applied consistently across multiple files. The new logic is straightforward (simple conditional assignment). The mechanical nature of replacing static options with dynamic method calls and the low behavioral impact keep complexity minimal. The main verification task is confirming consistency of the implementation across all callsites. Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/cli/create/route.go (1)
126-133: Consider consolidating duplicate toCreateOptions() implementations.This method duplicates the logic from
CreateSubcommandOptions.toCreateOptions()increate.go(lines 68-75). Both helpers have identical implementations.Consider one of these approaches:
Option 1: Make
CreateRouteSubcommandOptionsembed or referenceCreateSubcommandOptions:type CreateRouteSubcommandOptions struct { + CreateSubcommandOptions *CreateSubcommandOptions // PrintFlags holds options necessary for obtaining a printer PrintFlags *genericclioptions.PrintFlagsThen delegate to the shared helper:
func (o *CreateRouteSubcommandOptions) toCreateOptions() metav1.CreateOptions { - createOptions := metav1.CreateOptions{} - if o.DryRunStrategy == cmdutil.DryRunServer { - createOptions.DryRun = []string{metav1.DryRunAll} - } - - return createOptions + return o.CreateSubcommandOptions.toCreateOptions() }Option 2: Extract a standalone helper function that both can call:
func buildCreateOptions(dryRunStrategy cmdutil.DryRunStrategy) metav1.CreateOptions { createOptions := metav1.CreateOptions{} if dryRunStrategy == cmdutil.DryRunServer { createOptions.DryRun = []string{metav1.DryRunAll} } return createOptions }This reduces maintenance burden if the dry-run logic needs updating.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (13)
pkg/cli/create/build.go(1 hunks)pkg/cli/create/clusterquota.go(1 hunks)pkg/cli/create/create.go(2 hunks)pkg/cli/create/deploymentconfig.go(1 hunks)pkg/cli/create/identity.go(1 hunks)pkg/cli/create/imagestream.go(1 hunks)pkg/cli/create/imagestreamtag.go(1 hunks)pkg/cli/create/route.go(2 hunks)pkg/cli/create/routeedge.go(1 hunks)pkg/cli/create/routepassthrough.go(1 hunks)pkg/cli/create/routereenecrypt.go(1 hunks)pkg/cli/create/user.go(1 hunks)pkg/cli/create/user_identity_mapping.go(1 hunks)
🔇 Additional comments (13)
pkg/cli/create/routeedge.go (1)
139-139: LGTM! Consistent application of the new options builder pattern.The change correctly replaces the static
metav1.CreateOptions{}with a dynamic call too.CreateRouteSubcommandOptions.toCreateOptions(), aligning with the PR objective to pass dry-run options to the server. The existingDryRunClientcheck on line 138 is preserved, ensuring correct flow control.pkg/cli/create/deploymentconfig.go (1)
114-114: LGTM! Correctly applies the dynamic options pattern.The change appropriately replaces the literal
metav1.CreateOptions{}witho.CreateSubcommandOptions.toCreateOptions(), enabling server-side dry-run behavior. The dry-run strategy check on line 112 remains intact, maintaining the expected control flow.pkg/cli/create/imagestreamtag.go (1)
184-184: LGTM! Pattern applied correctly.The replacement of
metav1.CreateOptions{}witho.CreateSubcommandOptions.toCreateOptions()is consistent with the PR's objective to properly forward dry-run options to the API server. The existing dry-run handling on line 183 is preserved.pkg/cli/create/identity.go (1)
115-115: LGTM! Change aligns with the PR objective.The update correctly replaces the hardcoded
metav1.CreateOptions{}witho.CreateSubcommandOptions.toCreateOptions(), ensuring that dry-run options are properly communicated to the server. The dry-run strategy check on line 113 remains unchanged.pkg/cli/create/clusterquota.go (1)
140-140: LGTM! Consistent with the broader refactoring.The change from
metav1.CreateOptions{}too.CreateSubcommandOptions.toCreateOptions()properly implements the pattern for forwarding dry-run options to the API server. The control flow with the dry-run check on line 138 is maintained.pkg/cli/create/imagestream.go (1)
102-102: LGTM! Properly implements the options builder pattern.The substitution of
metav1.CreateOptions{}witho.CreateSubcommandOptions.toCreateOptions()correctly enables dynamic configuration of create options, particularly for dry-run behavior. The dry-run strategy check on line 100 is preserved.pkg/cli/create/routepassthrough.go (1)
105-105: LGTM! Change is consistent with the refactoring pattern.The replacement of
metav1.CreateOptions{}witho.CreateRouteSubcommandOptions.toCreateOptions()follows the established pattern across the PR, ensuring dry-run options are passed to the API server. The dry-run check on line 104 remains intact.pkg/cli/create/routereenecrypt.go (2)
154-154: LGTM! Pattern correctly applied.The change from
metav1.CreateOptions{}too.CreateRouteSubcommandOptions.toCreateOptions()aligns with the PR's goal of forwarding dry-run options to the server. The dry-run strategy check on line 153 is preserved.
1-161: Implementation verified; add tests for dry-run forwarding.All
toCreateOptions()implementations are correctly in place:
CreateSubcommandOptions.toCreateOptions()atpkg/cli/create/create.go:68CreateRouteSubcommandOptions.toCreateOptions()atpkg/cli/create/route.go:126Both properly set
DryRun: []string{metav1.DryRunAll}whenDryRunStrategy == cmdutil.DryRunServer. All 11 create subcommands (routereenecrypt, routepassthrough, routeedge, user, user_identity_mapping, imagestreamtag, identity, deploymentconfig, clusterquota, build, imagestream) have been updated to usetoCreateOptions()in their API calls.However, no test files exist in
pkg/cli/create/to verify dry-run options are properly forwarded to the API server. Add tests to validate this behavior.pkg/cli/create/user.go (1)
110-110: LGTM!The Create call now correctly uses dynamic CreateOptions from
toCreateOptions(), enabling server-side dry-run when--dry-run=serveris specified.pkg/cli/create/create.go (1)
68-75: LGTM!The helper correctly builds
CreateOptionswithDryRunset tometav1.DryRunAllwhenDryRunStrategyisDryRunServer, enabling server-side dry-run validation without creating resources.pkg/cli/create/build.go (1)
222-222: LGTM!The Create call now uses dynamic CreateOptions, consistent with the PR objectives to pass
--dry-runto the server.pkg/cli/create/user_identity_mapping.go (1)
134-134: LGTM!The Create call correctly uses dynamic CreateOptions from the helper method.
|
Overall idea sounds good to me. Nice catch! |
|
@liouk: The following test failed, say
Full PR test history. Your PR dashboard. 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 kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
Would be nice to somehow test this, not sure how much extra work that would be. |
@tchap I am currently writing some e2e tests in openshift/origin, I'll post an update here once I have a PR. |
liouk
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ardaguclu, liouk The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
Before merging this, we must wait for openshift/origin#30403 to have a failed run of the newly introduced test, in order to prove the issue exists. Once that happens, I'll merge this and rerun that test to prove the fix. /hold |
openshift-ci
bot
added
the
do-not-merge/hold
|
Pre-merge verfiied the PR, looks good as expected, detail see: https://issues.redhat.com/browse/OCPBUGS-62875?focusedId=28293571&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-28293571 |
|
/unhold |
openshift-ci
bot
removed
the
do-not-merge/hold
|
/verified by @wangke19 |
openshift-ci-robot
added
the
verified
|
@wangke19: This PR has been marked as verified by In 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-merge-bot
bot
merged commit 9735e9b
into
openshift:main
|
@liouk: Jira Issue OCPBUGS-62875: Some pull requests linked via external trackers have merged: The following pull request, linked via external tracker, has not merged:
All associated pull requests must be merged or unlinked from the Jira bug in order for it to move to the next state. Once unlinked, request a bug refresh with Jira Issue OCPBUGS-62875 has not been moved to the MODIFIED state. This PR is marked as verified. If the remaining PRs listed above are marked as verified before merging, the issue will automatically be moved to VERIFIED after all of the changes from the PRs are available in an accepted nightly payload. In 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. |
|
/cherry-pick releasel-4.20 |
|
@wangke19: cannot checkout In 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 kubernetes-sigs/prow repository. |
|
/cherry-pick release-4.20 |
|
@wangke19: new pull request created: #2121 In 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 kubernetes-sigs/prow repository. |
6 participants
The
createsubcommands ofocdo not pass along the--dry-runoption when used on the command-line. Therefore, the server-side dry run is ignored, and the resources get created.The fix provided in openshift/openshift-apiserver#511 was necessary for image streams, but we also need wiring on the
ocside.