Fix IoP metadata runtime evaluation and simplify URL caching #1138
+208
−9
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 GuideImplements runtime evaluation for IoP plugin metadata, simplifies URL resolution to always use the IoP‑aware helper instead of cached values, and makes the ping extension always registered but safely no‑ops when IoP is unavailable, with new tests covering IoP metadata and URL behavior across proxy lifecycle changes. Sequence diagram for IoP ping invocation with runtime IoP detectionsequenceDiagram
actor Admin
participant UI
participant ForemanServer
participant ForemanRhCloud_Ping as ForemanRhCloud_Ping
participant ForemanRhCloud as ForemanRhCloud
Admin->>UI: Trigger IoP ping (e.g. status check)
UI->>ForemanServer: HTTP request /rh_cloud/ping
ForemanServer->>ForemanRhCloud_Ping: ping
ForemanRhCloud_Ping->>ForemanRhCloud: with_iop_smart_proxy?
alt IoP smart proxy available
ForemanRhCloud-->>ForemanRhCloud_Ping: true
ForemanRhCloud_Ping->>ForemanRhCloud: iop_smart_proxy
ForemanRhCloud-->>ForemanRhCloud_Ping: smart_proxy
ForemanRhCloud_Ping->>ForemanRhCloud: iop_smart_proxy
ForemanRhCloud-->>ForemanRhCloud_Ping: smart_proxy
ForemanRhCloud_Ping->>ForemanRhCloud: iop_smart_proxy
ForemanRhCloud-->>ForemanRhCloud_Ping: smart_proxy
ForemanRhCloud_Ping->>ForemanRhCloud: iop_smart_proxy
ForemanRhCloud-->>ForemanRhCloud_Ping: smart_proxy
ForemanRhCloud_Ping->>ForemanRhCloud: iop_smart_proxy
ForemanRhCloud-->>ForemanRhCloud_Ping: smart_proxy
ForemanRhCloud_Ping->>ForemanRhCloud: iop_smart_proxy
ForemanRhCloud-->>ForemanRhCloud_Ping: smart_proxy
ForemanRhCloud_Ping->>ForemanRhCloud: iop_smart_proxy
ForemanRhCloud-->>ForemanRhCloud_Ping: smart_proxy
ForemanRhCloud_Ping->>ForemanRhCloud: iop_smart_proxy
ForemanRhCloud-->>ForemanRhCloud_Ping: smart_proxy
ForemanRhCloud_Ping->>ForemanRhCloud: iop_smart_proxy
ForemanRhCloud-->>ForemanRhCloud_Ping: smart_proxy
ForemanRhCloud_Ping->>ForemanRhCloud: iop_smart_proxy
ForemanRhCloud-->>ForemanRhCloud_Ping: smart_proxy
ForemanRhCloud_Ping->>ForemanRhCloud: iop_smart_proxy
ForemanRhCloud-->>ForemanRhCloud_Ping: smart_proxy
ForemanRhCloud_Ping->>ForemanRhCloud: iop_smart_proxy
ForemanRhCloud-->>ForemanRhCloud_Ping: smart_proxy
ForemanRhCloud_Ping->>ForemanRhCloud: iop_smart_proxy
ForemanRhCloud-->>ForemanRhCloud_Ping: smart_proxy
ForemanRhCloud_Ping->>ForemanRhCloud: iop_smart_proxy
ForemanRhCloud-->>ForemanRhCloud_Ping: smart_proxy
ForemanRhCloud_Ping->>ForemanRhCloud: iop_smart_proxy
ForemanRhCloud-->>ForemanRhCloud_Ping: smart_proxy
ForemanRhCloud_Ping->>ForemanRhCloud: iop_smart_proxy
ForemanRhCloud-->>ForemanRhCloud_Ping: smart_proxy
ForemanRhCloud_Ping->>ForemanRhCloud: iop_smart_proxy
ForemanRhCloud-->>ForemanRhCloud_Ping: smart_proxy
ForemanRhCloud_Ping->>ForemanRhCloud: iop_smart_proxy
ForemanRhCloud-->>ForemanRhCloud_Ping: smart_proxy
ForemanRhCloud_Ping->>ForemanRhCloud: iop_smart_proxy
ForemanRhCloud-->>ForemanRhCloud_Ping: smart_proxy
ForemanRhCloud_Ping->>ForemanRhCloud: iop_smart_proxy
ForemanRhCloud-->>ForemanRhCloud_Ping: smart_proxy
ForemanRhCloud_Ping->>ForemanRhCloud: iop_smart_proxy
ForemanRhCloud-->>ForemanRhCloud_Ping: smart_proxy
ForemanRhCloud_Ping->>ForemanRhCloud: iop_smart_proxy
ForemanRhCloud-->>ForemanRhCloud_Ping: smart_proxy
ForemanRhCloud_Ping->>ForemanRhCloud: iop_smart_proxy
ForemanRhCloud-->>ForemanRhCloud_Ping: smart_proxy
ForemanRhCloud_Ping->>ForemanRhCloud: iop_smart_proxy
ForemanRhCloud-->>ForemanRhCloud_Ping: smart_proxy
ForemanRhCloud_Ping->>ForemanRhCloud: iop_smart_proxy
ForemanRhCloud-->>ForemanRhCloud_Ping: smart_proxy
ForemanRhCloud_Ping->>ForemanRhCloud: iop_smart_proxy
ForemanRhCloud-->>ForemanRhCloud_Ping: smart_proxy
ForemanRhCloud_Ping->>ForemanRhCloud: iop_smart_proxy
ForemanRhCloud-->>ForemanRhCloud_Ping: smart_proxy
ForemanRhCloud_Ping->>ForemanRhCloud: iop_smart_proxy
ForemanRhCloud-->>ForemanRhCloud_Ping: smart_proxy
ForemanRhCloud_Ping->>ForemanRhCloud: iop_smart_proxy
ForemanRhCloud-->>ForemanRhCloud_Ping: smart_proxy
ForemanRhCloud_Ping->>ForemanRhCloud: iop_smart_proxy
ForemanRhCloud-->>ForemanRhCloud_Ping: smart_proxy
ForemanRhCloud_Ping->>ForemanRhCloud: iop_smart_proxy
ForemanRhCloud-->>ForemanRhCloud_Ping: smart_proxy
ForemanRhCloud_Ping->>ForemanRhCloud: iop_smart_proxy
ForemanRhCloud-->>ForemanRhCloud_Ping: smart_proxy
ForemanRhCloud_Ping->>ForemanRhCloud: iop_smart_proxy
ForemanRhCloud-->>ForemanRhCloud_Ping: smart_proxy
ForemanRhCloud_Ping->>ForemanRhCloud: iop_smart_proxy
ForemanRhCloud-->>ForemanRhCloud_Ping: smart_proxy
ForemanRhCloud_Ping->>ForemanRhCloud: iop_smart_proxy
ForemanRhCloud-->>ForemanRhCloud_Ping: smart_proxy
ForemanRhCloud_Ping->>ForemanRhCloud: iop_smart_proxy
ForemanRhCloud-->>ForemanRhCloud_Ping: smart_proxy
ForemanRhCloud_Ping->>ForemanRhCloud: iop_smart_proxy
ForemanRhCloud-->>ForemanRhCloud_Ping: smart_proxy
ForemanRhCloud_Ping->>ForemanRhCloud: iop_smart_proxy
ForemanRhCloud-->>ForemanRhCloud_Ping: smart_proxy
ForemanRhCloud_Ping->>ForemanRhCloud: iop_smart_proxy
ForemanRhCloud-->>ForemanRhCloud_Ping: smart_proxy
ForemanRhCloud_Ping->>ForemanRhCloud: iop_smart_proxy
ForemanRhCloud-->>ForemanRhCloud_Ping: smart_proxy
ForemanRhCloud_Ping->>ForemanRhCloud: iop_smart_proxy
ForemanRhCloud-->>ForemanRhCloud_Ping: smart_proxy
ForemanRhCloud_Ping->>ForemanRhCloud: iop_smart_proxy
ForemanRhCloud-->>ForemanRhCloud_Ping: smart_proxy
ForemanRhCloud_Ping->>ForemanRhCloud: iop_smart_proxy
ForemanRhCloud-->>ForemanRhCloud_Ping: smart_proxy
ForemanRhCloud_Ping->>ForemanRhCloud_Ping: iop_smart_proxy_url
ForemanRhCloud_Ping-->>ForemanRhCloud_Ping: cache or ForemanRhCloud.iop_smart_proxy&.url
ForemanRhCloud_Ping->>ForemanRhCloud_Ping: ping_services
ForemanRhCloud_Ping-->>ForemanServer: IoP ping result
else No IoP smart proxy
ForemanRhCloud-->>ForemanRhCloud_Ping: false
ForemanRhCloud_Ping-->>ForemanServer: {}
end
ForemanServer-->>UI: HTTP response
UI-->>Admin: Show IoP status
Sequence diagram for runtime evaluation of IoP plugin metadatasequenceDiagram
participant ForemanCore as Foreman_Core
participant MetadataRegistry as AppMetadataRegistry
participant ForemanRhCloudPlugin as ForemanRhCloud_Plugin
participant ForemanRhCloud as ForemanRhCloud
ForemanRhCloudPlugin->>MetadataRegistry: register foreman_rh_cloud iop: lambda
MetadataRegistry-->>ForemanRhCloudPlugin: registration complete
loop On each metadata request
ForemanCore->>MetadataRegistry: get foreman_rh_cloud metadata
MetadataRegistry->>ForemanRhCloud: with_iop_smart_proxy? via lambda
ForemanRhCloud-->>MetadataRegistry: current IoP availability
MetadataRegistry-->>ForemanCore: metadata iop value reflects current IoP state
end
Updated class diagram for ForemanRhCloud URL resolution and Ping behaviorclassDiagram
class ForemanRhCloud {
+env_or_on_premise_url(env_var_name)
+base_url()
+cert_base_url()
+legacy_insights_url()
+verify_ssl_method()
+with_iop_smart_proxy?()
+iop_smart_proxy()
}
class ForemanRhCloud_Plugin {
+register()
}
class AppMetadataRegistry {
+register(key, metadata_hash)
}
class ForemanRhCloud_Ping {
+iop_smart_proxy_url()
+service_urls()
+ping()
+status()
+ping_services()
+exception_watch(result, blk)
}
ForemanRhCloud_Plugin --> AppMetadataRegistry : registers_metadata
ForemanRhCloud_Plugin --> ForemanRhCloud_Ping : register_ping_extension
ForemanRhCloud_Ping --> ForemanRhCloud : uses_with_iop_smart_proxy?
ForemanRhCloud_Ping --> ForemanRhCloud : uses_iop_smart_proxy
AppMetadataRegistry --> ForemanRhCloud : calls_with_iop_smart_proxy?_lambda
ForemanRhCloud_Ping --> ForemanRhCloud : uses_urls
ForemanRhCloud ..> ForemanRhCloud_Ping : shared_namespace
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- Consider dropping the memoization on
iop_smart_proxy_url(or adding a reset hook) so it stays consistent with the new runtime IoP detection—right now it can cache anilor stale URL even though other IoP-related URLs are evaluated dynamically on each call.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider dropping the memoization on `iop_smart_proxy_url` (or adding a reset hook) so it stays consistent with the new runtime IoP detection—right now it can cache a `nil` or stale URL even though other IoP-related URLs are evaluated dynamically on each call.
## Individual Comments
### Comment 1
<location> `test/unit/foreman_rh_cloud_iop_metadata_test.rb:79-88` </location>
<code_context>
+ describe 'URL methods' do
</code_context>
<issue_to_address>
**suggestion (testing):** Add similar ENV and IoP precedence tests for cert_base_url and legacy_insights_url
Current precedence tests for `SATELLITE_RH_CLOUD_URL` / IoP and `base_url` look good, but there are no analogous tests for `cert_base_url` (`SATELLITE_CERT_RH_CLOUD_URL`) and `legacy_insights_url` (`SATELLITE_LEGACY_INSIGHTS_URL`). Please add tests that:
- Verify `cert_base_url` uses `SATELLITE_CERT_RH_CLOUD_URL` when set and no IoP proxy exists.
- Verify `legacy_insights_url` uses `SATELLITE_LEGACY_INSIGHTS_URL` when set and no IoP proxy exists.
- Verify that when both the ENV var and an IoP proxy are present, the IoP URL takes precedence for both helpers, matching the `base_url` behavior.
This keeps all three URL helpers consistent and better protected against regressions.
Suggested implementation:
```ruby
describe 'URL methods' do
test 'base_url returns cloud URL when no IoP smart proxy exists' do
assert_equal 'https://cloud.redhat.com', ForemanRhCloud.base_url
end
test 'cert_base_url returns cloud URL when no IoP smart proxy exists' do
assert_equal 'https://cert.cloud.redhat.com', ForemanRhCloud.cert_base_url
end
test 'legacy_insights_url returns cloud URL when no IoP smart proxy exists' do
assert_equal 'https://cert-api.access.redhat.com', ForemanRhCloud.legacy_insights_url
end
test 'cert_base_url prefers SATELLITE_CERT_RH_CLOUD_URL when set and no IoP smart proxy exists' do
old_env = ENV['SATELLITE_CERT_RH_CLOUD_URL']
ENV['SATELLITE_CERT_RH_CLOUD_URL'] = 'https://env-cert.cloud.test'
begin
assert_equal 'https://env-cert.cloud.test', ForemanRhCloud.cert_base_url
ensure
ENV['SATELLITE_CERT_RH_CLOUD_URL'] = old_env
end
end
test 'legacy_insights_url prefers SATELLITE_LEGACY_INSIGHTS_URL when set and no IoP smart proxy exists' do
old_env = ENV['SATELLITE_LEGACY_INSIGHTS_URL']
ENV['SATELLITE_LEGACY_INSIGHTS_URL'] = 'https://env-legacy.insights.test'
begin
assert_equal 'https://env-legacy.insights.test', ForemanRhCloud.legacy_insights_url
ensure
ENV['SATELLITE_LEGACY_INSIGHTS_URL'] = old_env
end
end
test 'cert_base_url prefers IoP proxy URL over SATELLITE_CERT_RH_CLOUD_URL when both are present' do
old_env = ENV['SATELLITE_CERT_RH_CLOUD_URL']
ENV['SATELLITE_CERT_RH_CLOUD_URL'] = 'https://env-cert.cloud.test'
ForemanRhCloud.stubs(:iop_proxy_url).returns('https://iop-cert.cloud.test')
begin
assert_equal 'https://iop-cert.cloud.test', ForemanRhCloud.cert_base_url
ensure
ENV['SATELLITE_CERT_RH_CLOUD_URL'] = old_env
ForemanRhCloud.unstub(:iop_proxy_url)
end
end
test 'legacy_insights_url prefers IoP proxy URL over SATELLITE_LEGACY_INSIGHTS_URL when both are present' do
old_env = ENV['SATELLITE_LEGACY_INSIGHTS_URL']
ENV['SATELLITE_LEGACY_INSIGHTS_URL'] = 'https://env-legacy.insights.test'
ForemanRhCloud.stubs(:iop_proxy_url).returns('https://iop-legacy.insights.test')
begin
assert_equal 'https://iop-legacy.insights.test', ForemanRhCloud.legacy_insights_url
ensure
ENV['SATELLITE_LEGACY_INSIGHTS_URL'] = old_env
ForemanRhCloud.unstub(:iop_proxy_url)
end
end
```
These tests assume:
1. The application reads the environment variables `SATELLITE_CERT_RH_CLOUD_URL` and `SATELLITE_LEGACY_INSIGHTS_URL` directly via `ENV[...]`.
2. `ForemanRhCloud.iop_proxy_url` is the method used by `cert_base_url` and `legacy_insights_url` to discover the IoP proxy URL, and Mocha is available for `stubs`/`unstub`.
If the existing `base_url` precedence tests use a different helper (e.g. a factory for an IoP proxy or a wrapper for environment handling), mirror that pattern instead of stubbing `iop_proxy_url`, to keep the style consistent across all three URL helpers.
</issue_to_address>
### Comment 2
<location> `test/unit/foreman_rh_cloud_iop_metadata_test.rb:129-142` </location>
<code_context>
+ ENV.delete('SATELLITE_RH_CLOUD_URL')
+ end
+
+ test 'URL methods update when IoP smart proxy is created' do
+ # Initially returns cloud URL
+ assert_equal 'https://cloud.redhat.com', ForemanRhCloud.base_url
+
+ # Create an IoP smart proxy
+ create_iop_proxy
+
+ # Should now return IoP URL
+ assert_equal 'https://iop.example.com', ForemanRhCloud.base_url
+ end
+
+ test 'URL methods update when IoP smart proxy is destroyed' do
+ # Create an IoP smart proxy
+ proxy = create_iop_proxy
</code_context>
<issue_to_address>
**suggestion (testing):** Extend dynamic IoP creation/destruction tests to cert_base_url and legacy_insights_url
Since `cert_base_url` and `legacy_insights_url` now also use `env_or_on_premise_url` and should reflect IoP availability, these tests should assert their behavior alongside `base_url`.
In the "created" test, also assert initial cloud values for all three helpers, then the IoP URL for all three after proxy creation. In the "destroyed" test, assert the IoP URL for all three before destruction, then the cloud URLs again after the proxy is destroyed. This keeps the tests consistent with the PR’s stated behavior for all URL helpers.
```suggestion
test 'URL methods update when IoP smart proxy is created' do
# Initially returns cloud URLs
assert_equal 'https://cloud.redhat.com', ForemanRhCloud.base_url
assert_equal 'https://cert.cloud.redhat.com', ForemanRhCloud.cert_base_url
assert_equal 'https://cert-api.access.redhat.com', ForemanRhCloud.legacy_insights_url
# Create an IoP smart proxy
create_iop_proxy
# Should now return IoP URL for all helpers
assert_equal 'https://iop.example.com', ForemanRhCloud.base_url
assert_equal 'https://iop.example.com', ForemanRhCloud.cert_base_url
assert_equal 'https://iop.example.com', ForemanRhCloud.legacy_insights_url
end
test 'URL methods update when IoP smart proxy is destroyed' do
# Create an IoP smart proxy
proxy = create_iop_proxy
# Initially returns IoP URL for all helpers
assert_equal 'https://iop.example.com', ForemanRhCloud.base_url
assert_equal 'https://iop.example.com', ForemanRhCloud.cert_base_url
assert_equal 'https://iop.example.com', ForemanRhCloud.legacy_insights_url
# Destroy the IoP smart proxy
proxy.destroy
# Should now return cloud URLs again
assert_equal 'https://cloud.redhat.com', ForemanRhCloud.base_url
assert_equal 'https://cert.cloud.redhat.com', ForemanRhCloud.cert_base_url
assert_equal 'https://cert-api.access.redhat.com', ForemanRhCloud.legacy_insights_url
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
jeremylenz
force-pushed
the
fix-iop-metadata-runtime-evaluation
branch
from
4f1e3c2 to
6b9f600
Compare
- Wrap IoP metadata in lambda for runtime evaluation to prevent stale state during app initialization - Remove URL instance variable caching as env_or_on_premise_url is already IoP-aware - Add nil safety to ping methods - Add comprehensive unit tests for IoP metadata and URL methods Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
- Add ENV variable precedence tests for cert_base_url and legacy_insights_url - Extend dynamic IoP creation/destruction tests to verify all URL helpers - Fix memoization in Ping class for iop_smart_proxy_url to ensure runtime IoP detection Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
jeremylenz
force-pushed
the
fix-iop-metadata-runtime-evaluation
branch
from
6b9f600 to
164a07b
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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?
ForemanRhCloud.with_iop_smart_proxy?in a lambda for ForemanContext app metadata registration. This ensures the UI responds immediately when switching between IoP and non-IoP.AI slop below, but I'm gonna hand-write the manual testing steps.
IoP Metadata Runtime Evaluation:
with_iop_smart_proxy?check in lambda for plugin metadata registrationSimplified URL Caching:
base_url,cert_base_url, andlegacy_insights_urlenv_or_on_premise_urlwhich is already IoP-awarePing Extension Improvements:
{}when no IoP)iop_smart_proxy_urlwith safe navigation operatorpingmethod when no IoP proxy existsConsiderations taken when implementing this change?
What are the testing steps for this pull request?
Unit Tests:
Manual Testing:
Check out this PR and the Foreman PR --> theforeman/foreman#10814
Start Foreman with IoP smart proxy
Infrastructure > About: Backend system status should show "advisor" and "vulnerability"
Main nav: Insights should have 'Recommendations' and 'Vulnerability' sub-items.
Both pages should load Scalprum components (you can tell because you see the "loading..." text)
Host details page should have Recommendations and Vulnerability tabs
Recommendations tab should load the Scalprum component
Now, go to Infrastructure > Smart Proxies and DELETE the IoP smart proxy
The following changes should be IMMEDIATE:
Infrastructure > About: Backend system status will NOT have "advisor" and "vulnerability"
Main nav: Insights should have 'Recommendations' and 'Inventory Upload'
Host details page should have only 'Recommendations', NOT 'Vulnerabilities'
Should NOT load Scalprum components anywhere
Now, Infrastructure > Smart Proxies > Create a new smart proxy with url
https://localhost:24443Everything should immediately go back to how it was in steps 1-6.