CONSOLE-4987: Conditionally render Guided Tour based on capability and disable it for Console E2E tests#15926
CONSOLE-4987: Conditionally render Guided Tour based on capability and disable it for Console E2E tests#15926krishagarwal278 wants to merge 1 commit intoopenshift:mainfrom
Conversation
|
@krishagarwal278: This pull request references CONSOLE-4987 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 "4.22.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. |
📝 WalkthroughWalkthroughAdds a new capability feature flag, Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
|
@krishagarwal278: This pull request references CONSOLE-4987 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 "4.22.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. |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/public/components/app.tsx (1)
169-196: Backend capability nameGuidedTourFeatureis not defined in the OpenShift API and will never be emitted.The code searches for a capability named
GuidedTourFeature, but the OpenShift API enum (vendor/github.com/openshift/api/operator/v1/types_console.go) only definesLightspeedButtonandGettingStartedBanneras validConsoleCapabilityNamevalues. The kubebuilder validation enforces this enum restriction, meaning:
- The backend will never emit a capability named
GuidedTourFeatureguidedTourCapabilitywill always beundefined- The logic
guidedTourCapability?.visibility?.state !== 'Disabled'will always evaluate totrue- The flag
CONSOLE_CAPABILITY_GUIDEDTOUR_IS_ENABLEDwill always be enabled, regardless of intentAdd
GuidedTourFeatureto theConsoleCapabilityNameenum in the OpenShift API types, update the kubebuilder validation enum constraint, and ensure the backend handles emitting this capability.
725889f to
f26a5b3
Compare
|
@krishagarwal278: This pull request references CONSOLE-4987 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 "4.22.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. |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/public/components/app.tsx (1)
175-196: Backend API sync required: 'GuidedTourFeature' capability not registered.The backend API definition (
vendor/github.com/openshift/api/operator/v1/types_console.go:138) only registers"LightspeedButton"and"GettingStartedBanner"as valid capabilities. The frontend is searching for"GuidedTourFeature"(line 176), which doesn't exist in the API spec and will never be returned by the server.The condition
guidedTourCapability?.visibility?.state !== 'Disabled'evaluates totruewhenguidedTourCapabilityis undefined, accidentally enabling the tour for backwards compatibility. However, this bypasses API validation and will break if the backend enforces the capability enum.The API types must be updated to include
"GuidedTourFeature"in the ConsoleCapabilityName enum (line 138 ofvendor/github.com/openshift/api/operator/v1/types_console.go), or this frontend code needs to be reverted.
|
/retest |
ff9fa0b to
9feb299
Compare
|
/retest |
|
Checked on cluster launched against the pr, the GuidedTour capability is set as Enabled by default in console operator, there is Guided Tour when login console. Set GuidedTour capability as Disabled in console operator. Login console again, there is not Guided Tour. |
|
@yanpzhan: 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. |
|
/assign @jhadvig |
|
@krishagarwal278: GitHub didn't allow me to assign the following users: jseseCCS. Note that only openshift members with read permissions, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time. 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. |
|
one minor suggestion in 1 file. otherwise, LGTM! |
|
/label docs-approved |
frontend/public/components/app.tsx
Outdated
| dispatch( | ||
| setFlag( | ||
| FLAGS.CONSOLE_CAPABILITY_GUIDEDTOUR_IS_ENABLED, | ||
| // Default to enabled if capability is not explicitly set (backwards compatibility) |
There was a problem hiding this comment.
Minor: change “backwards compatibility” → “backward compatibility” for style consistency?
There was a problem hiding this comment.
no sure why the comment is needed ?
frontend/public/components/app.tsx
Outdated
| setFlag( | ||
| FLAGS.CONSOLE_CAPABILITY_GUIDEDTOUR_IS_ENABLED, | ||
| // Default to enabled if capability is not explicitly set (backwards compatibility) | ||
| guidedTourCapability?.visibility?.state !== 'Disabled', |
There was a problem hiding this comment.
Lets follow the pattern from above
guidedTourCapability?.visibility?.state === 'Enabled',
frontend/public/components/app.tsx
Outdated
| dispatch( | ||
| setFlag( | ||
| FLAGS.CONSOLE_CAPABILITY_GUIDEDTOUR_IS_ENABLED, | ||
| // Default to enabled if capability is not explicitly set (backwards compatibility) |
There was a problem hiding this comment.
no sure why the comment is needed ?
10df2be to
c27e3f5
Compare
…ndencies to vendor openshift/api with gudiedTour capability
c27e3f5 to
9774f79
Compare
|
/label px-approved |
|
/retest |
jhadvig
left a comment
There was a problem hiding this comment.
Thanks @krishagarwal278 for the update.
The code looks good but I dont see the code responsible for conditionally rendering the GuiderdTour component based on the flag.
Also we need to check if all the e2e tests (besides GuidedTour test itself) set the flag to disabled.
|
Hi @jhadvig, thanks for the review! The conditional rendering is currently handled at the extension manifest level via |
|
ok, nice. I see that in the GuidedTour component. In that case we should be fine /lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jhadvig, jseseCCS, krishagarwal278, logonoff 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 |
|
/verified by yanpzhan |
|
@yanpzhan: 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. |
|
/retest |
1 similar comment
|
/retest |
|
@krishagarwal278: 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. |
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.