Skip to content

Fix os type response isuserdefined - #8128

Merged
DaanHoogland merged 1 commit into
apache:mainfrom
isupersky:fix-OSType-response-isuserdefined
Oct 25, 2023
Merged

Fix os type response isuserdefined#8128
DaanHoogland merged 1 commit into
apache:mainfrom
isupersky:fix-OSType-response-isuserdefined

Conversation

@isupersky

@isuperskyisupersky commented Oct 21, 2023

Copy link
Copy Markdown
Contributor

Description

This PR resolves issue #8127. Changing the datatype of isUserDefined of class GuestOSResponse from String to Boolean.

Fixes: #8127

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

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?

@boring-cyborg

Copy link
Copy Markdown

Congratulations on your first Pull Request and welcome to the Apache CloudStack community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/cloudstack/blob/main/CONTRIBUTING.md)
Here are some useful points:

@shwstppr

shwstppr commented Oct 21, 2023

Copy link
Copy Markdown
Contributor

@isupersky can you please properly rebase your PR branch with 4.18 branch?

@isupersky
isupersky changed the base branch from 4.18 to mainOctober 21, 2023 20:10
@codecov

codecovBot commented Oct 21, 2023

Copy link
Copy Markdown

Codecov Report

Merging #8128 (4065ef3) into main (99ded81) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@ Coverage Diff @@## main #8128 +/- ##
============================================
+ Coverage 29.22% 29.25% +0.02% - Complexity 30490 30520 +30 
============================================
Files 5101 5101 Lines 358817 358817 Branches 52362 52362 ============================================
+ Hits 104861 104958 +97 + Misses 239558 239458 -100 - Partials 14398 14401 +3 
FlagCoverage Δ
simulator-marvin-tests25.22% <100.00%> (+0.03%)⬆️
uitests4.84% <ø> (ø)
unit-tests14.51% <0.00%> (ø)

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

FilesCoverage Δ
...pache/cloudstack/api/response/GuestOSResponse.java68.18% <ø> (ø)
...src/main/java/com/cloud/api/ApiResponseHelper.java47.76% <100.00%> (ø)

... and 27 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@shwstppr

Copy link
Copy Markdown
Contributor

@blueorangutan package

@blueorangutan

Copy link
Copy Markdown

@shwstppr 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
Copy Markdown

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

@weizhouapacheweizhouapache left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

CODE LGTM

@weizhouapache

Copy link
Copy Markdown
Member

@blueorangutan test

@blueorangutan

Copy link
Copy Markdown

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

@blueorangutan

Copy link
Copy Markdown

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

TestResultTime (s)Test File

@shwstppr

Copy link
Copy Markdown
Contributor

@weizhouapache@DaanHoogland@harikrishna-patnala@rohityadavcloud should this also go into 4.18?

@DaanHooglandDaanHoogland 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.

clgtm

@DaanHoogland

DaanHoogland commented Oct 24, 2023

Copy link
Copy Markdown
Contributor

@weizhouapache@DaanHoogland@harikrishna-patnala@rohityadavcloud should this also go into 4.18?

this seems a regression @shwstppr , on 4.18 the field is already a bool.

@shwstppr

Copy link
Copy Markdown
Contributor

my bad @DaanHoogland. Probably referred wrong original PR

@weizhouapache

Copy link
Copy Markdown
Member

@weizhouapache@DaanHoogland@harikrishna-patnala@rohityadavcloud should this also go into 4.18?

this seems a regression @shwstppr , on 4.18 the field is already a bool.

ooo
It has already been fixed in #2632, and reverted in #6979

@DaanHoogland

Copy link
Copy Markdown
Contributor

yes @weizhouapache ,
this asks for some kind of test (beit unit or integration)

@shwstpprshwstppr added this to the 4.19.0.0 milestone Oct 24, 2023
@DaanHoogland

Copy link
Copy Markdown
Contributor

tested:
without this PR

 {
"description": "Windows XP SP3 (32-bit)",
"fordisplay": true,
"id": "e6561287-6db0-11ee-9ca5-1e003a000ac4",
"isuserdefined": "false",
"name": "Windows XP SP3 (32-bit)",
"oscategoryid": "e64d1a63-6db0-11ee-9ca5-1e003a000ac4",
"oscategoryname": "Windows"
}

with

 {
"description": "Windows XP SP3 (32-bit)",
"fordisplay": true,
"id": "246184aa-727c-11ee-9c7e-1e00ac0002c4",
"isuserdefined": false,
"name": "Windows XP SP3 (32-bit)",
"oscategoryid": "2452d76f-727c-11ee-9c7e-1e00ac0002c4",
"oscategoryname": "Windows"
}

merging

@DaanHoogland
DaanHoogland merged commit 8fe13f9 into apache:mainOct 25, 2023
@boring-cyborg

Copy link
Copy Markdown

Awesome work, congrats on your first merged pull request!

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

OSType response: isuserdefined as string not bool

5 participants

@isupersky@shwstppr@blueorangutan@weizhouapache@DaanHoogland