Repository sync tasks fail with 429 Too Many Requests because IoP vmaas reposcan syncs are requested when one is already in progress#1210
Conversation
![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 1 issue, and left some high level feedback:
- In
plan,Katello::Repository.find(repo_id)will raise if the repository is missing; if that’s a realistic scenario it may be worth rescuing and logging a clearer message rather than letting the action error out unexpectedly. - The
InsightsCloud.vmaas_reposcan_sync_urlhelper hardcodes the sync path; consider centralizing the full IoP VMaaS endpoint path (including any versioned/api/v1/...segments) there so it can be reused consistently and adjusted in one place if the gateway changes.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `plan`, `Katello::Repository.find(repo_id)` will raise if the repository is missing; if that’s a realistic scenario it may be worth rescuing and logging a clearer message rather than letting the action error out unexpectedly.
- The `InsightsCloud.vmaas_reposcan_sync_url` helper hardcodes the sync path; consider centralizing the full IoP VMaaS endpoint path (including any versioned `/api/v1/...` segments) there so it can be reused consistently and adjusted in one place if the gateway changes.
## Individual Comments
### Comment 1
<location path="lib/insights_cloud.rb" line_range="48-49" />
<code_context>
'enable_cloud_remediations'
end
+
+ def self.vmaas_reposcan_sync_url
+ ForemanRhCloud.iop_smart_proxy.url + '/api/vmaas-reposcan/sync'
+ end
end
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Consider more robust URL construction to avoid double slashes and missing trailing slashes.
String concatenation here assumes a specific trailing-slash convention on `ForemanRhCloud.iop_smart_proxy.url`, which can easily change and silently produce malformed URLs (`//api/...` or missing `/`). Prefer a URL-joining helper (e.g., `URI.join` or a dedicated method) so the path is composed correctly regardless of configuration.
Suggested implementation:
```ruby
def self.vmaas_reposcan_sync_url
URI.join(ForemanRhCloud.iop_smart_proxy.url.to_s, '/api/vmaas-reposcan/sync').to_s
end
end
```
If `URI` is not already loaded in this file or a common initializer, add `require 'uri'` near the top of `lib/insights_cloud.rb` (or in a shared place) so `URI.join` is available.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| def self.vmaas_reposcan_sync_url | ||
| ForemanRhCloud.iop_smart_proxy.url + '/api/vmaas-reposcan/sync' |
There was a problem hiding this comment.
suggestion (bug_risk): Consider more robust URL construction to avoid double slashes and missing trailing slashes.
String concatenation here assumes a specific trailing-slash convention on ForemanRhCloud.iop_smart_proxy.url, which can easily change and silently produce malformed URLs (//api/... or missing /). Prefer a URL-joining helper (e.g., URI.join or a dedicated method) so the path is composed correctly regardless of configuration.
Suggested implementation:
def self.vmaas_reposcan_sync_url
URI.join(ForemanRhCloud.iop_smart_proxy.url.to_s, '/api/vmaas-reposcan/sync').to_s
end
endIf URI is not already loaded in this file or a common initializer, add require 'uri' near the top of lib/insights_cloud.rb (or in a shared place) so URI.join is available.
…an#1180) * Fix VmaasReposcanSync errors preventing parent task success When VmaasReposcanSync receives an error from the IoP service (especially 429 during concurrent syncs), the action was re-raising exceptions which caused parent Katello repository sync tasks to fail. This fix removes the raise statements and allows the rescue_strategy_for_self Skip to work correctly, ensuring repository syncs succeed even when reposcan requests fail. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> (cherry picked from commit 38bc0e2)
I don't see a version bump in the commits, is that intentional? |
Yes, until we get a green build or approval, I did not want to bump the version w/o getting a review from you or @jeremylenz |
Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
| HTTP_TOO_MANY_REQUESTS = 429 | ||
|
|
||
| # Subscribe to Katello repository sync hook action, if available | ||
| def self.subscribe |
There was a problem hiding this comment.
Why is this being cherry-picked to this branch? Without the subscription here, the class is never invoked. That means (as far as I can tell) we are cherry-picking the VmaasReposcanSync feature itself. Is that what's intended?
|
@Odilhao we should probably close this PR, right? |
5 participants
What are the changes introduced in this pull request?
Considerations taken when implementing this change?
Cherrypicking d667c27#diff-fdc5302b48f87097e991ddc150d69efc91c5792ae784d30c519e83fd2d9bea70 failed because there is no script folder on this branch, I deleted and kept only the ones related to vmass
What are the testing steps for this pull request?