Refactor local advisor engine config with SmartProxy-based detection #1049
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's GuideThis PR refactors the Insights On-Premise (IoP) detection from static ENV and setting flags to dynamic SmartProxy feature checks, renames the legacy use_local_advisor_engine flag to use_iop_mode across the stack, and propagates these changes through services, jobs, routes, views, plugin registration, and tests to preserve backward compatibility while removing manual ENV management. ER diagram for SmartProxy and Insights On-Premise detectionerDiagram
SMART_PROXY ||--o{ FOREMAN_RH_CLOUD : provides
SMART_PROXY {
string url
string[] features
}
FOREMAN_RH_CLOUD {
bool use_iop_mode
}
Class diagram for ForemanRhCloud IoP detection refactorclassDiagram
class ForemanRhCloud {
+on_premise_url()
+env_or_on_premise_url(env_var_name)
+proxy_setting()
+proxy_string()
+marked_foreman_host()
+legacy_insights_ca()
+cloud_url_validator()
+with_insights_smartproxy()? <<new>>
+insights_smartproxy <<new>>
}
class SmartProxy {
+with_features(feature)
+exists?()
+first()
+url
}
ForemanRhCloud --> SmartProxy : uses for IoP detection
Class diagram for AdvisorEngineConfigController API changeclassDiagram
class AdvisorEngineConfigController {
+show()
}
AdvisorEngineConfigController : - use_iop_mode (renamed from use_local_advisor_engine)
AdvisorEngineConfigController --> ForemanRhCloud : uses with_insights_smartproxy?
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
nofaralfasi
force-pushed
the
35683_cu
branch
2 times, most recently
from
a360d46 to
3d4c8ee
Compare
nofaralfasi
commented
Jul 31, 2025
nofaralfasi
commented
Jul 31, 2025
Comment on lines
-8
to
-9
| port = ENV['ADVISOR_ENGINE_PORT'] || "24443" | ||
| ENV['ADVISOR_ENGINE_URL'] || "https://localhost:#{port}" |
There was a problem hiding this comment.
@ShimShtein I want to confirm whether it's safe to remove these lines.
I see that this method is still being used here.
Should we keep it as a fallback for default values?
If we do need to keep it, the implementation will have to be updated due to recent changes in the env_or_on_premise_url method.
There was a problem hiding this comment.
Seems OK. In on-prem we want the smart-proxy to force-drive the URLs
Replace configuration-based IoP detection with SmartProxy feature detection. The system now automatically detects Insights on Premise mode by checking for SmartProxy instances with the 'Insights' feature instead of relying on environment variables and settings.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @nofaralfasi - I've reviewed your changes - here's some feedback:
- Consider memoizing the insights_smartproxy lookup to avoid a database query on every with_insights_smartproxy? call and reduce performance overhead.
- Add handling for the case where multiple SmartProxy instances expose the 'Insights' feature—either warn or fail fast—to prevent ambiguity in which URL is chosen.
- Provide a deprecated alias for with_local_advisor_engine? to smooth the transition for any code paths or plugins that still rely on the old method name.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider memoizing the insights_smartproxy lookup to avoid a database query on every with_insights_smartproxy? call and reduce performance overhead.
- Add handling for the case where multiple SmartProxy instances expose the 'Insights' feature—either warn or fail fast—to prevent ambiguity in which URL is chosen.
- Provide a deprecated alias for with_local_advisor_engine? to smooth the transition for any code paths or plugins that still rely on the old method name.
## Individual Comments
### Comment 1
<location> `lib/foreman_rh_cloud/engine.rb:128` </location>
<code_context>
+ SmartProxy.with_features('Insights').exists?
+ end
+
+ def self.insights_smartproxy
+ SmartProxy.with_features('Insights').first
end
</code_context>
<issue_to_address>
Ambiguity if multiple SmartProxies with 'Insights' feature exist.
Currently, the method returns the first matching SmartProxy, which may cause unpredictable results if multiple exist. Consider enforcing uniqueness or raising an error when more than one is found.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+17
to
18
| const isLocalAdvisorEngine = advisorEngineConfig?.use_iop_mode; | ||
| return isLocalAdvisorEngine; |
There was a problem hiding this comment.
suggestion (code-quality): Inline variable that is immediately returned (inline-immediately-returned-variable)
Suggested change
| const isLocalAdvisorEngine = advisorEngineConfig?.use_iop_mode; | |
| return isLocalAdvisorEngine; | |
| return advisorEngineConfig?.use_iop_mode; | |
Explanation
Something that we often see in people's code is assigning to a result variableand then immediately returning it.
Returning the result directly shortens the code and removes an unnecessary
variable, reducing the mental load of reading the function.
Where intermediate variables can be useful is if they then get used as a
parameter or a condition, and the name can act like a comment on what the
variable represents. In the case where you're returning it from a function, the
function name is there to tell you what the result is, so the variable name
is unnecessary.
There was a problem hiding this comment.
@nofaralfasi The AI is right here, I'll leave it up to you if you want to change it.
ShimShtein
reviewed
Jul 31, 2025
| @@ -23,7 +23,7 @@ def plan | |||
| end | |||
|
|
|||
| def run | |||
| output[:status] = _('The scheduled process is disabled because this Foreman is configured with the use_local_advisor_engine option.') if ForemanRhCloud.with_local_advisor_engine? | |||
| output[:status] = _('The scheduled process is disabled because this Foreman is configured with a local Insights SmartProxy.') if ForemanRhCloud.with_insights_smartproxy? | |||
There was a problem hiding this comment.
Suggested change
| output[:status] = _('The scheduled process is disabled because this Foreman is configured with a local Insights SmartProxy.') if ForemanRhCloud.with_insights_smartproxy? | |
| output[:status] = _('The scheduled process is disabled because this Foreman is configured with a local Insights Smart Proxy.') if ForemanRhCloud.with_insights_smartproxy? |
I think it should be two words
| @@ -34,7 +34,7 @@ def plan | |||
| end | |||
|
|
|||
| def run | |||
| output[:status] = _('The scheduled process is disabled because this Foreman is configured with the use_local_advisor_engine option.') if ForemanRhCloud.with_local_advisor_engine? | |||
| output[:status] = _('The scheduled process is disabled because this Foreman is configured with a local Insights SmartProxy.') if ForemanRhCloud.with_insights_smartproxy? | |||
There was a problem hiding this comment.
Suggested change
| output[:status] = _('The scheduled process is disabled because this Foreman is configured with a local Insights SmartProxy.') if ForemanRhCloud.with_insights_smartproxy? | |
| output[:status] = _('The scheduled process is disabled because this Foreman is configured with a local Insights Smart Proxy.') if ForemanRhCloud.with_insights_smartproxy? |
| @@ -35,7 +35,7 @@ def plan_org_sync(org) | |||
| end | |||
|
|
|||
| def run | |||
| output[:status] = _('The scheduled process is disabled because this Foreman is configured with the use_local_advisor_engine option.') if ForemanRhCloud.with_local_advisor_engine? | |||
| output[:status] = _('The scheduled process is disabled because this Foreman is configured with a local Insights SmartProxy.') if ForemanRhCloud.with_insights_smartproxy? | |||
There was a problem hiding this comment.
Suggested change
| output[:status] = _('The scheduled process is disabled because this Foreman is configured with a local Insights SmartProxy.') if ForemanRhCloud.with_insights_smartproxy? | |
| output[:status] = _('The scheduled process is disabled because this Foreman is configured with a local Insights Smart Proxy.') if ForemanRhCloud.with_insights_smartproxy? |
Comment on lines
+17
to
18
| const isLocalAdvisorEngine = advisorEngineConfig?.use_iop_mode; | ||
| return isLocalAdvisorEngine; |
There was a problem hiding this comment.
@nofaralfasi The AI is right here, I'll leave it up to you if you want to change it.
ehelms
reviewed
Jul 31, 2025
| @@ -7,7 +7,7 @@ class AdvisorEngineConfigController < ::Api::V2::BaseController | |||
| api :GET, "/rh_cloud/advisor_engine_config", N_("Show if system is configured to use local iop-advisor-engine.") | |||
| def show | |||
| render json: { | |||
| use_local_advisor_engine: ForemanRhCloud.with_local_advisor_engine?, | |||
| use_iop_mode: ForemanRhCloud.with_insights_smartproxy?, | |||
There was a problem hiding this comment.
Can we sub iop for insights ?
@ShimShtein @vkrizan I am thinking we should change the smart-proxy feature as well. We have an inconsistency in nomenclature.
There was a problem hiding this comment.
The proxy feature is Insights. We can converge to local_insights_mode WDYT?
There was a problem hiding this comment.
I think my point is being missed. I am wanting to remove the use of the word Insights in favor of just using iop
There was a problem hiding this comment.
I don't mind either. I'm with Shim, that the feature is name Insights, like the opensource counterpart. The "on Prem" can be implied for Foreman/Satellite.
There was a problem hiding this comment.
Proposed change -- RedHatInsights/iop-gateway#17
jeremylenz
reviewed
Aug 1, 2025
| @@ -121,8 +121,12 @@ def self.register_scheduled_task(task_class, cronline) | |||
| end | |||
| end | |||
|
|
|||
| def self.with_local_advisor_engine? | |||
| SETTINGS.dig(:foreman_rh_cloud, :use_local_advisor_engine) || false | |||
| def self.with_insights_smartproxy? | |||
There was a problem hiding this comment.
Smart Proxy is two words.
Suggested change
| def self.with_insights_smartproxy? | |
| def self.with_insights_smart_proxy? |
| SmartProxy.with_features('Insights').exists? | ||
| end | ||
|
|
||
| def self.insights_smartproxy |
There was a problem hiding this comment.
Suggested change
| def self.insights_smartproxy | |
| def self.insights_smart_proxy |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What are the changes introduced in this pull request?
Replace configuration-based IoP detection with SmartProxy feature detection. The system now automatically detects Insights on Premise mode by checking for SmartProxy instances with the 'Insights' feature instead of relying on environment variables and settings.
Considerations taken when implementing this change?
The main goal was to preserve backward compatibility while moving from static environment variable configuration to dynamic detection based on SmartProxy settings. Instead of relying on manually set ENV vars, the system now determines IoP mode by querying the SmartProxy, which improves accuracy and makes it easier to maintain.
There's a small trade-off in performance (a DB query instead of reading a cached variable), but it removes the need for operational ENV management. Renaming the API field from
use_local_advisor_enginetouse_iop_modemakes the purpose clearer.I'm still considering whether to keep the
with_local_advisor_engine?method and mark it as deprecated, mainly to ensure older code paths continue to work during the transition.What are the testing steps for this pull request?
To Do
Summary by Sourcery
Introduce SmartProxy-based detection for Insights On-Prem and refactor plugin registration while renaming the legacy use_local_advisor_engine flag to use_iop_mode
New Features:
Enhancements:
Tests:
Summary by Sourcery
Replace static local advisor engine configuration with dynamic SmartProxy-based detection for Insights on-premise and rename related flags to use_iop_mode.
Enhancements:
Tests: