Skip to content

Prevent NPE if guest OS mapping is missing while prioritizing hosts - #2577

Merged
DaanHoogland merged 1 commit into
apache:4.11from
khos2ow:fix-missing-guest-os-mapping
Apr 20, 2018
Merged

Prevent NPE if guest OS mapping is missing while prioritizing hosts#2577
DaanHoogland merged 1 commit into
apache:4.11from
khos2ow:fix-missing-guest-os-mapping

Conversation

@khos2ow

@khos2ow khos2ow commented Apr 17, 2018

Copy link
Copy Markdown
Contributor

Description

When starting an existing VM and trying to prioritize hosts, if Guest OS mapping has been removed from that host, there will be a NullPointerException which prevents the VM to be started successfully.

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)

GitHub Issue/PRs

Screenshots (if appropriate):

How Has This Been Tested?

Checklist:

  • I have read the CONTRIBUTING document.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
    Testing
  • I have added tests to cover my changes.
  • All relevant new and existing integration tests have passed.
  • A full integration testsuite with all test that can run on my environment has passed.

@blueorangutan package

@blueorangutan

Copy link
Copy Markdown

@khos2ow a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

// Determine the guest OS category of the template
String templateGuestOSCategory = getTemplateGuestOSCategory(template);

if (Strings.isNullOrEmpty(templateGuestOSCategory)) {

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.

if you return a null value in getTemplateGuestOSCategory method, why not simply check templateGuestOSCategory == null?

Is it possible to have an empty or blank guestOSCategory.name?
BTW: Strings.isNullOrEmpty will return false for blanks.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Technically templateGuestOSCategory == null would be enough, and I cannot think of a way getTemplateGuestOSCategory returning empty string unless the database record integrity has been changed, that's why I used Strings.isNullOrEmpty rather rather than a simple == null check. (which might be slightly overkill at the same time!)

@blueorangutan

Copy link
Copy Markdown

Packaging result: ✔centos6 ✔centos7 ✔debian. JID-1938

@yadvr

yadvr commented Apr 19, 2018

Copy link
Copy Markdown
Member

@khos2ow can you change base branch to 4.11?

@khos2ow
khos2ow force-pushed the fix-missing-guest-os-mapping branch from 82b7c2e to 26b8041 Compare April 19, 2018 14:51
@khos2ow
khos2ow changed the base branch from master to 4.11 April 19, 2018 14:51
@khos2ow

khos2ow commented Apr 19, 2018

Copy link
Copy Markdown
Contributor Author

@rhtyd done.

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

LGTM

@yadvr yadvr added this to the 4.11.1.0 milestone Apr 19, 2018
@yadvr

yadvr commented Apr 19, 2018

Copy link
Copy Markdown
Member

@blueorangutan package

@blueorangutan

Copy link
Copy Markdown

@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

@blueorangutan

Copy link
Copy Markdown

Packaging result: ✔centos6 ✖centos7 ✖debian. JID-1952

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

LGTM

@DaanHoogland
DaanHoogland merged commit 995775d into apache:4.11 Apr 20, 2018
@DaanHoogland

Copy link
Copy Markdown
Contributor

sorry for merging this. I did not check the integration tests. hope i will be forgiven one day.

@yadvr

yadvr commented Apr 20, 2018

Copy link
Copy Markdown
Member

LGTM, given the changes on discretion I'm okay to accept/merge this as travis passed.

@khos2ow
khos2ow deleted the fix-missing-guest-os-mapping branch April 20, 2018 14:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants