-
Notifications
You must be signed in to change notification settings - Fork 50
Register foreman_rh_cloud app metadata #1107
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Reviewer's GuideThis PR introduces a new IOP-based feature flag for the foreman_rh_cloud plugin by registering metadata on plugin load and refactors all components to use a new useIopConfig hook that reads from context metadata (falling back to the API only if needed). Class diagram for updated ConfigHooks and useIopConfigclassDiagram
class useIopConfig {
+result: boolean (from context metadata)
+skipApiRequest: boolean
+advisorEngineConfig: object (from API)
+return: boolean (context or API)
}
useIopConfig --|> useAPI : uses
useIopConfig --|> useForemanContext : uses
class useAPI {
+response: object
}
class useForemanContext {
+metadata: object
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey there - I've reviewed your changes - here's some feedback:
- Consider centralizing the fallback default for the IOP flag inside your useIopConfig hook so consumers don't have to replicate the
?? truelogic in the ForemanColumnExtensions and elsewhere. - Rename useIopConfig (and its return value) to something like useIopSmartProxyEnabled or isIopSmartProxy to better convey that it checks IoP smart proxy availability.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider centralizing the fallback default for the IOP flag inside your useIopConfig hook so consumers don't have to replicate the `?? true` logic in the ForemanColumnExtensions and elsewhere.
- Rename useIopConfig (and its return value) to something like useIopSmartProxyEnabled or isIopSmartProxy to better convey that it checks IoP smart proxy availability.
## Individual Comments
### Comment 1
<location> `webpack/common/Hooks/ConfigHooks.js:13` </location>
<code_context>
+ const skipApiRequest = result !== undefined;
const { response: advisorEngineConfig } = useAPI(
- 'get',
+ skipApiRequest ? null : 'get',
ADVISOR_ENGINE_CONFIG_PATH,
{
</code_context>
<issue_to_address>
**issue (bug_risk):** Passing `null` as the HTTP method may cause issues in some API hooks.
Please confirm that `useAPI` can handle a `null` method without errors, or refactor to ensure a valid method is always provided.
</issue_to_address>
### Comment 2
<location> `webpack/ForemanColumnExtensions/index.js:77` </location>
<code_context>
{
columnName: 'cves_count',
title: __('Total CVEs'),
+ // eslint-disable-next-line camelcase
+ isRelevant: context => context?.metadata?.foreman_rh_cloud.iop ?? true,
wrapper: hostDetails => <CVECountCell hostDetails={hostDetails} />,
weight: 2600,
</code_context>
<issue_to_address>
**suggestion:** Defaulting to `true` for `isRelevant` may unintentionally show the column.
If you want the column hidden when `iop` is undefined, use `?? false` instead.
```suggestion
isRelevant: context => context?.metadata?.foreman_rh_cloud.iop ?? false,
```
</issue_to_address>
### Comment 3
<location> `webpack/common/Hooks/ConfigHooks.js:21` </location>
<code_context>
// eslint-disable-next-line camelcase
- return advisorEngineConfig?.use_iop_mode;
+ return skipApiRequest ? result : advisorEngineConfig?.use_iop_mode;
};
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Return value may be inconsistent in type between context and API response.
Normalize the return value to ensure consistent type and semantics, preventing subtle bugs from type mismatches.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Github was not working right so I closed this and opened #1116 |
What are the changes introduced in this pull request?
Requires theforeman/foreman#10713
In foreman_rh_cloud there are a quite many places where we need to know if it's an IoP setup or not. On the backend this is easy: we'd just call
ForemanRhCloud.with_iop_smart_proxy?. But from React/JS, it's a bit tricky and there are a couple methods:Neither of these is ideal, for various reasons
With this change, the
.with_iop_smart_proxy?value is added toForemanContextand we can use it anywhere without making an API call.Additionally, since IoP is now more than just Advisor engine, I've taken the liberty of renaming our React hook.
Considerations taken when implementing this change?
I didn't remove the endpoint completely. IoP services may still need to call it some day..?
What are the testing steps for this pull request?
get an IoP setup
Load pages that make this request and observe lack of request with the PR checked out
Summary by Sourcery
Register the foreman_rh_cloud iop flag in the plugin metadata registry and refactor the configuration hook and related components to use the new useIopConfig hook for conditional rendering and API request skipping
New Features:
Enhancements: