OCPBUGS-76280: Next button is disabled on Optional configurations page#3384
Conversation
openshift-ci-robot
added
jira/valid-reference
|
@ElayAharoni: This pull request references Jira Issue OCPBUGS-76280, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ElayAharoni 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 |
openshift-ci
bot
added
the
size/XS
📝 WalkthroughWalkthroughRemoves pullSecret from the form model and UI; integrates Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Form as OptionalConfigurationsStep (Form)
participant Hook as usePullSecret
participant API as InfraEnv API
rect rgba(200,200,255,0.5)
User->>Form: click Next / submit
Form->>Hook: read defaultPullSecret
alt creating InfraEnv
Form->>API: createInfraEnv(createParams including defaultPullSecret ?? '')
else updating existing InfraEnv
Form->>API: updateInfraEnv(updateParams without pullSecret)
end
API-->>Form: response
Form-->>User: navigate next
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/OptionalConfigurationsStep.tsx`:
- Around line 140-145: The editPullSecret state is incorrectly initialized from
the async usePullSecret result (defaultPullSecret) causing it to be true on
first render; add a useEffect that watches defaultPullSecret and calls
setEditPullSecret(!defaultPullSecret) (or explicitly set false when
defaultPullSecret is truthy) to update editPullSecret after the fetch resolves;
keep the initial useState as a safe default (e.g., true or false) and ensure the
effect only runs when defaultPullSecret changes to avoid flipping user edits,
referencing editPullSecret, setEditPullSecret, usePullSecret, and
defaultPullSecret.
🧹 Nitpick comments (1)
libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/OptionalConfigurationsStep.tsx (1)
140-140: DuplicateusePullSecret()call triggers a redundant API fetch.
usePullSecret()is already invoked insidePullSecretSync(line 121). Calling it again here creates a second independent hook instance with its own state and a second network request to fetch the pull secret. Consider lifting the result from a shared source (e.g., pass it down, or use a context/shared state) rather than calling the hook twice.
libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/OptionalConfigurationsStep.tsx
Outdated
Show resolved
Hide resolved
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@ElayAharoni: This pull request references Jira Issue OCPBUGS-76280, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: 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: GitHub didn't allow me to request PR reviews from the following users: bmanzari. Note that only openshift-assisted members and repo collaborators can review this PR, and authors cannot review their own PRs. 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 kubernetes-sigs/prow repository. |
ElayAharoni
force-pushed
the
Next-button-is-disabled-on-Optional-configurations-page
branch
from
c9a1560 to
64034a3
Compare
There was a problem hiding this comment.
Could we not just use PullSecret.tsx for this? Isn't the desired behavior basically the same?
Signed-off-by: Elay Aharoni <elayaha@gmail.com>
ElayAharoni
force-pushed
the
Next-button-is-disabled-on-Optional-configurations-page
branch
from
64034a3 to
32da038
Compare
openshift-ci
bot
added
size/M
openshift-merge-robot
added
the
needs-rebase
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/OptionalConfigurationsStep.tsx`:
- Line 201: The form currently allows submitting an InfraEnv with an empty pull
secret because usePullSecret() can be undefined or '' and the current
isNextDisabled check doesn't verify defaultPullSecret; update the component to
require a truthy pull secret before enabling Next and to avoid sending an empty
string as InfraEnvCreateParams.pullSecret: change the payload assignment for
pullSecret to use defaultPullSecret only when truthy (do not default to ''), and
update the isNextDisabled logic (the check used to enable the Next button) to
include a `!defaultPullSecret` check (or `!Boolean(defaultPullSecret)`) so Next
remains disabled until usePullSecret() resolves to a non-empty value.
🧹 Nitpick comments (3)
libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/OptionalConfigurationsStep.tsx (3)
68-68: Remove commented-out code.Dead commented-out line
// pullSecret,should be removed to keep the codebase clean.Proposed fix
return { - // pullSecret, ...(values.sshPublicKey && { sshAuthorizedKey: values.sshPublicKey }),
225-229:handleNextis recreated on every render inside the render-prop.This is minor but
handleNextis defined inline in the Formik render prop without memoization. SincesubmitFormidentity is stable from Formik, this is functionally fine, but worth noting if you see unnecessary re-renders ofClusterWizardFooter.
147-157:initialValuesis recreated on every render, which can cause Formik to reset.
initialValuesis declared inside the component body without memoization. If the parent re-renders, a new object reference is created each time. WithenableReinitializeoff (default) this is typically harmless, but wrapping it inuseMemoor hoisting it outside the component is a small defensive improvement.
| openshiftVersion: cluster.openshiftVersion, | ||
| cpuArchitecture: DEFAULT_CPU_ARCHITECTURE, | ||
| imageType: DISCONNECTED_IMAGE_TYPE, | ||
| pullSecret: defaultPullSecret ?? '', |
There was a problem hiding this comment.
Risk of creating InfraEnv with an empty pull secret.
usePullSecret() returns undefined on initial render and '' on fetch failure. Since isNextDisabled on line 239 doesn't gate on defaultPullSecret being available, a user can submit the form before the pull secret resolves (or after it fails), sending an empty string to InfraEnvCreateParams.pullSecret.
Disable "Next" until a valid pull secret is available:
Proposed fix
- isNextDisabled={!isValid || !cluster}
+ isNextDisabled={!isValid || !cluster || !defaultPullSecret}This also covers the error/empty-data cases from usePullSecret (which sets ''), since an empty string is falsy.
Also applies to: 239-239
🤖 Prompt for AI Agents
In
`@libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/OptionalConfigurationsStep.tsx`
at line 201, The form currently allows submitting an InfraEnv with an empty pull
secret because usePullSecret() can be undefined or '' and the current
isNextDisabled check doesn't verify defaultPullSecret; update the component to
require a truthy pull secret before enabling Next and to avoid sending an empty
string as InfraEnvCreateParams.pullSecret: change the payload assignment for
pullSecret to use defaultPullSecret only when truthy (do not default to ''), and
update the isNextDisabled logic (the check used to enable the Next button) to
include a `!defaultPullSecret` check (or `!Boolean(defaultPullSecret)`) so Next
remains disabled until usePullSecret() resolves to a non-empty value.
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 (3)
libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/OptionalConfigurationsStep.tsx (3)
48-67:⚠️ Potential issue | 🟡 MinorLeftover
pullSecretinDEFAULT_INITIAL_VALUESnot in type.
pullSecretwas removed fromOptionalConfigurationsFormValues(lines 48–54) but still appears on line 58. This is either a TypeScript error (excess property on an object literal) or dead code. Same issue ininfraEnvToFormValueson line 78.Proposed fix
const DEFAULT_INITIAL_VALUES: OptionalConfigurationsFormValues = { sshPublicKey: '', - pullSecret: '', enableProxy: false, httpProxy: '', httpsProxy: '',And in
infraEnvToFormValues:const infraEnvToFormValues = ( infraEnv: InfraEnv, - storedPullSecret?: string, ): OptionalConfigurationsFormValues => ({ ...DEFAULT_INITIAL_VALUES, sshPublicKey: infraEnv.sshAuthorizedKey ?? '', - pullSecret: storedPullSecret ?? '', enableProxy: !!(infraEnv.proxy?.httpProxy || infraEnv.proxy?.httpsProxy),
233-236:⚠️ Potential issue | 🟠 Major
values.pullSecretisundefined— type no longer includespullSecret.Since
pullSecretwas removed fromOptionalConfigurationsFormValues,values.pullSecretis alwaysundefined. This silently sets the wizard context's pull secret toundefinedon both the update path (line 233) and the create path (line 254), which may break downstream consumers expecting a string.If the pull secret is no longer managed through the form, these calls should use
defaultPullSecret(or be removed if no longer needed).Proposed fix
- setDisconnectedFormPullSecret(values.pullSecret); + setDisconnectedFormPullSecret(defaultPullSecret ?? '');Apply the same change on line 254.
72-90:⚠️ Potential issue | 🟡 MinorRemove unused
infraEnvToFormValuesfunction.The function is never called anywhere in the codebase. Its JSDoc indicates it was intended to rehydrate form values from an existing infraEnv, but the form currently always initializes from
DEFAULT_INITIAL_VALUES, meaning any edits on back-navigation would be lost. Either remove this dead code or restore the call to rehydrate form state.
🤖 Fix all issues with AI agents
In
`@libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/OptionalConfigurationsStep.tsx`:
- Around line 192-202: The form's initialValues in
OptionalConfigurationsStep.tsx are hardcoded, causing user inputs to be lost
when revisiting; replace the static initialValues with values derived from
disconnectedInfraEnv (using the existing infraEnvToFormValues helper or
equivalent) and/or compute initialValues from disconnectedInfraEnv before
passing to Formik, and enable Formik's enableReinitialize so the form rehydrates
when disconnectedInfraEnv changes; update references to
OptionalConfigurationsFormValues and ensure hostsNetworkConfigurationType and
other fields are populated from infraEnvToFormValues/disconnectedInfraEnv rather
than empty defaults.
🧹 Nitpick comments (1)
libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/OptionalConfigurationsStep.tsx (1)
104-106: Remove commented-out code.Line 105 has a leftover
// pullSecret,comment. Clean it up.
libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/OptionalConfigurationsStep.tsx
Show resolved
Hide resolved
|
@ElayAharoni: The following tests failed, say
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. |
|
@ElayAharoni: This pull request references Jira Issue OCPBUGS-76280. The bug has been updated to no longer refer to the pull request using the external bug tracker. 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. |
4 participants
https://issues.redhat.com/browse/OCPBUGS-76280
the next button is disabled on purpose because although the step is called optional configurations, the pull secret fields is required.
so therefore if there is no actual pull secret than the user will have to enter one before clicking on next.
the behaviour was that initially the checkbox was unchecked if there is no pull secret so it looked like nothing is wrong and the next button is disabled due to a bug.
i have fixed it now so that the the user will be able to see that the pull secret is required and it will be more informative.
before:
after:
Summary by CodeRabbit
Bug Fixes
Improvements