Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

mgmt: add back serviceip in ManagementServerResponse#10891

Merged
Pearl1594 merged 3 commits intoapache:4.20apache/cloudstack:4.20from
weizhouapache:4.20-add-back-serviceipweizhouapache/cloudstack:4.20-add-back-serviceipCopy head branch name to clipboard
May 21, 2025
Merged

mgmt: add back serviceip in ManagementServerResponse#10891
Pearl1594 merged 3 commits intoapache:4.20apache/cloudstack:4.20from
weizhouapache:4.20-add-back-serviceipweizhouapache/cloudstack:4.20-add-back-serviceipCopy head branch name to clipboard

Conversation

@weizhouapache
Copy link
Contributor

@weizhouapache weizhouapache commented May 19, 2025

Description

The PR #9773 changed the serviceip to ipaddress in the API response, which broke backward compatibility.

Although a test failure has been fixed in #10501 , I think it would be better to add it back.

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):

How Has This Been Tested?

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

@codecov
Copy link

codecov bot commented May 19, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 0% with 7 lines in your changes missing coverage. Please review.

Project coverage is 16.14%. Comparing base (5444261) to head (dfd1c46).
Report is 2 commits behind head on 4.20.

Files with missing lines Patch % Lines
...udstack/api/response/ManagementServerResponse.java 0.00% 6 Missing ⚠️
...ain/java/com/cloud/api/query/QueryManagerImpl.java 0.00% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff            @@
##               4.20   #10891   +/-   ##
=========================================
  Coverage     16.13%   16.14%           
- Complexity    13239    13241    +2     
=========================================
  Files          5656     5656           
  Lines        497579   497586    +7     
  Branches      60333    60333           
=========================================
+ Hits          80297    80312   +15     
+ Misses       408333   408324    -9     
- Partials       8949     8950    +1
Flag Coverage Δ
uitests 4.00% <ø> (ø)
unittests 16.99% <0.00%> (+<0.01%) ⬆️

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

☔ View full report in Codecov by Sentry.
📢 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.

abh1sar
abh1sar approved these changes May 19, 2025
View reviewed changes
Copy link
Collaborator

@abh1sar abh1sar left a comment

Choose a reason for hiding this comment

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

LGTM

@yadvr yadvr added this to the 4.20.1 milestone May 20, 2025
@yadvr yadvr added the Severity:Critical Critical bug label May 20, 2025
yadvr
yadvr approved these changes May 20, 2025
View reviewed changes
Copy link
Member

@yadvr yadvr left a comment

Choose a reason for hiding this comment

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

LGTM - since this fixes API regression, we should consider this as critical for 4.20.1

@yadvr
Copy link
Member

yadvr commented May 20, 2025

@weizhouapache can you also look at failing github action smoketests?
@blueorangutan package

@blueorangutan
Copy link

blueorangutan commented May 20, 2025

@rohityadavcloud 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
Copy link

blueorangutan commented May 20, 2025

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

@weizhouapache
Copy link
Contributor Author

weizhouapache commented May 20, 2025

@weizhouapache can you also look at failing github action smoketests?

thanks @rohityadavcloud

the remaining test failures are addressed by #10875

@weizhouapache weizhouapache marked this pull request as ready for review May 20, 2025 09:12
@weizhouapache
Copy link
Contributor Author

weizhouapache commented May 20, 2025

@blueorangutan test

@blueorangutan
Copy link

blueorangutan commented May 20, 2025

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

@DaanHoogland
Copy link
Contributor

DaanHoogland commented May 20, 2025

@weizhouapache , can we somehow mark it as deprecated? (ltgm, btw)

@weizhouapache
Copy link
Contributor Author

weizhouapache commented May 20, 2025

@weizhouapache , can we somehow mark it as deprecated? (ltgm, btw)

@DaanHoogland
yes, I will add Deprecated annotation, but I am not sure if it makes differences.

@DaanHoogland
Copy link
Contributor

DaanHoogland commented May 20, 2025

@weizhouapache , can we somehow mark it as deprecated? (ltgm, btw)

@DaanHoogland yes, I will add Deprecated annotation, but I am not sure if it makes differences.

I think it would be good to add it to the description that using ipaddress instead is advised.

@weizhouapache
Copy link
Contributor Author

weizhouapache commented May 20, 2025

@weizhouapache , can we somehow mark it as deprecated? (ltgm, btw)

@DaanHoogland yes, I will add Deprecated annotation, but I am not sure if it makes differences.

I think it would be good to add it to the description that using ipaddress instead is advised.

that's already added. @DaanHoogland

I added Deprecated annotation as well

@DaanHoogland
Copy link
Contributor

DaanHoogland commented May 20, 2025

that's already added. @DaanHoogland

sorry, reviewed from the wild (not at desk)

I added Deprecated annotation as well

great, I don’t think we can do better.

@blueorangutan
Copy link

blueorangutan commented May 21, 2025

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

Test Result Time (s) Test File

@Pearl1594 Pearl1594 merged commit d0dc6d8 into apache:4.20 May 21, 2025
22 of 26 checks passed
@DaanHoogland DaanHoogland deleted the 4.20-add-back-serviceip branch May 22, 2025 09:55
dhslove pushed a commit to ablecloud-team/ablestack-cloud that referenced this pull request Jun 19, 2025
@weizhouapache @dhslove
mgmt: add back serviceip in ManagementServerResponse (apache#10891)
ea431e6 
* mgmt: add back serviceip in ManagementServerResponse

* api: add Deprecated annotation to serviceip
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@yadvr yadvr yadvr approved these changes

@Pearl1594 Pearl1594 Pearl1594 approved these changes

@DaanHoogland DaanHoogland Awaiting requested review from DaanHoogland

+1 more reviewer

@abh1sar abh1sar abh1sar approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

component:api Severity:Critical Critical bug

Projects

ACS 4.20.1
Status: Done

Milestone

4.20.1

Development

Successfully merging this pull request may close these issues.

6 participants

@weizhouapache @yadvr @blueorangutan @DaanHoogland @Pearl1594 @abh1sar
Morty Proxy This is a proxified and sanitized view of the page, visit original site.