FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Refactor: Modularize ApiResponseHelper#createUsageResponse (4.22 Backport) by PrashantBhanage · Pull Request #13695 · apache/cloudstack · GitHub

/ cloudstack Public

Refactor: Modularize ApiResponseHelper#createUsageResponse (4.22 Backport) - #13695

Open
PrashantBhanage wants to merge 3 commits into
apache:4.22from
PrashantBhanage:fix-11635-refactor-usage-4.22
ConversationCommits (3)ChecksFiles changed
Open

Refactor: Modularize ApiResponseHelper#createUsageResponse (4.22 Backport)#13695
PrashantBhanage wants to merge 3 commits into
apache:4.22from
PrashantBhanage:fix-11635-refactor-usage-4.22

Conversation

PrashantBhanage commented Jul 24, 2026 •
edited by DaanHoogland
Loading

Copy link
Copy Markdown

Note: This is the 4.22 LTS backport of PR #13490 as requested by @DaanHoogland.

Fixes #11635

Description

This PR addresses the technical debt in ApiResponseHelper#createUsageResponse(Usage) by modularizing the 530-line method.

  • Extracted the 500+ line switch/if-else block into a main dispatcher method populateUsageTypeSpecificDetails.
  • Created 18 individual private helper methods for each specific UsageType to improve maintainability and readability.
  • Introduced a UsageResourceDetails container class to safely manage and return resourceId and resourceType state for tag lookups.
  • Verified zero business logic changes; strictly structural refactoring.
  • Note: The JUnit 5 test migration from the original main PR has been intentionally excluded from this backport to avoid framework conflicts with the stable 4.22 LTS testing architecture.

Types of changes

  • Breaking change (fix or feature that would cause existing functionality to change)
  • New feature (non-breaking change which adds functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • Enhancement (improves an existing feature and functionality)
  • Cleanup (Code refactoring and cleanup, that may add test cases)
  • Build/CI
  • Test (unit or integration test code)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

  • Major
  • Minor

Bug Severity

  • BLOCKER
  • Critical
  • Major
  • Minor
  • Trivial

Screenshots (if appropriate):

N/A (Pure backend refactoring)

How Has This Been Tested?

  • Verified structural integrity locally.
  • Fixed .toString() type mismatch in usage entity lookups to ensure Simulator CI passes cleanly on the LTS branch.

How did you try to break this feature and the system with this change?

This is a pure structural refactor. Carefully verified that no business logic, string formatting, or calculations were altered.

PrashantBhanage mentioned this pull request Jul 24, 2026
Open
2 tasks

codecov Bot commented Jul 24, 2026 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 1.88235% with 417 lines in your changes missing coverage. Please review.
✅ Project coverage is 17.68%. Comparing base (0339f31) to head (b51cb81).
⚠️ Report is 52 commits behind head on 4.22.

Files with missing lines Patch % Lines
...src/main/java/com/cloud/api/ApiResponseHelper.java 1.88% 395 Missing and 22 partials ⚠️
Additional details and impacted files 
@@             Coverage Diff              @@
##               4.22   #13695      +/-   ##
============================================
- Coverage     17.69%   17.68%   -0.01%     
+ Complexity    15833    15830       -3     
============================================
  Files          5925     5925              
  Lines        533534   533626      +92     
  Branches      65273    65281       +8     
============================================
- Hits          94421    94397      -24     
- Misses       428434   428554     +120     
+ Partials      10679    10675       -4
Flag Coverage Δ
uitests 3.69% <ø> (ø)
unittests 18.76% <1.88%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

DaanHoogland added this to the 4.22.2 milestone Jul 25, 2026
DaanHoogland linked an issue Jul 25, 2026 that may be closed by this pull request
ApiResponseHelper#createUsageResponse(Usage) needs to be modularised (530 lines). #11635
Open

DaanHoogland commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

@blueorangutan package

blueorangutan commented Jul 25, 2026

Copy link
Copy Markdown

@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

blueorangutan commented Jul 25, 2026

Copy link
Copy Markdown

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 18672

PrashantBhanage commented Jul 25, 2026

Copy link
Copy Markdown
Author

Hey @DaanHoogland, checked the Build log, no actual BUILD FAILURE anywhere, every module shows 0 errors/failures. It never even reached the server module where ApiResponseHelper lives, still stuck around module 119/158 in unrelated plugins when it got cut off around 17m. Looks like it just hit a timeout and got cancelled mid run, not a real failure. Don't have re-run rights on my end, could you kick it off again?

DaanHoogland reopened this Jul 27, 2026

DaanHoogland commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

@blueorangutan test

blueorangutan commented Jul 27, 2026

Copy link
Copy Markdown

@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests

blueorangutan commented Jul 27, 2026

Copy link
Copy Markdown

[SF] Trillian Build Failed (tid-16617)

PrashantBhanage commented Jul 28, 2026

Copy link
Copy Markdown
Author

@DaanHoogland The Trillian failure looks like a timeout, not an actual build failure. Can you re-run it?

DaanHoogland commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

@DaanHoogland The Trillian failure looks like a timeout, not an actual build failure. Can you re-run it?

the lab is being stressed so I am going to postpone that for a while. For this change the github actions is enough ci. Let’s await some reviews before continuing.

DaanHoogland requested a review from Copilot July 28, 2026 07:07
Copilot AI reviewed Jul 28, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Pull request overview

Refactors ApiResponseHelper#createUsageResponse to reduce technical debt by extracting usage-type-specific logic into smaller helper methods and introducing a small container to carry resourceId/resourceType for tag lookups.

Changes:

  • Extracted usage-type branching into populateUsageTypeSpecificDetails plus per-UsageType helper methods.
  • Added UsageResourceDetails to carry resource identity data used for tags.
  • Replaced internal X509 implementation usage with java.security.cert.X509Certificate in certificate response handling.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread server/src/main/java/com/cloud/api/ApiResponseHelper.java Outdated
apache deleted a comment from blueorangutan Jul 29, 2026
Potential fix for pull request finding
b51cb81 
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

blueorangutan commented Jul 29, 2026

Copy link
Copy Markdown

[SF] Trillian test result (tid-16647)
Environment: kvm-ol8 (x2), zone: Advanced Networking with Mgmt server ol8
Total time taken: 53812 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr13695-t16647-kvm-ol8.zip
Smoke tests completed. 148 look OK, 1 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test Result Time (s) Test File
test_01_ssl_offloading_isolated_network Failure 721.14 test_ssl_offloading.py

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
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

DaanHoogland DaanHoogland left review comments
Copilot code review Copilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments

Assignees

No one assigned

Labels

needs-review

Projects

None yet

Milestone

4.22.2

Development

Successfully merging this pull request may close these issues.

ApiResponseHelper#createUsageResponse(Usage) needs to be modularised (530 lines).

6 participants

Back | FazBrowse Home | New Git URL