CLOUDSTACK-10362: Change the "getXXX" method names to "isXXX". by Kui-Liu · Pull Request #2600 · apache/cloudstack

Kui-Liu · Apr 24, 2018

These Boolean-return methods are named "getXXX",
but other Boolean-return methods are named "isXXX", such as the following two methods.
They will return boolean values, rename them as "isXXX" should be more clear than "getXXX".

api/src/main/java/org/apache/cloudstack/api/command/admin/vlan/ListVlanIpRangesCmd.java

public Boolean getForVirtualNetwork() {
      return forVirtualNetwork;
  }

api/src/main/java/org/apache/cloudstack/api/command/user/address/ListPublicIpAddressesCmd.java

public Boolean isForVirtualNetwork() {
     return forVirtualNetwork;
 }

These Boolean-return methods are named "getXXX", but other Boolean-return methods are named "isXXX", such as the following two methods. They will return boolean values, rename them as "isXXX" should be more clear than "getXXX".

DaanHoogland

I am fine wit most of the changes from a coding perspective, though I don't agree with all the choices made but my biggest concern is that some paring ort code generation relying on nameconventions will break with this. So I think intensive testing is required.

DaanHoogland · Apr 27, 2018

api/src/main/java/org/apache/cloudstack/api/command/user/vm/DeployVMCmd.java

    }

-    public Boolean getDisplayVm() {
+    public Boolean isDisplayVm() {


this one is strange. it means to say shouldDisplayVm() but is can live with it

DaanHoogland · Apr 27, 2018

api/src/main/java/org/apache/cloudstack/api/command/user/vpc/UpdateVPCCmd.java

    }

-    public Boolean getDisplayVpc() {
+    public Boolean isDisplayVpc() {


same here; shouldDisplayVpc()??

DaanHoogland · Apr 27, 2018

api/src/main/java/org/apache/cloudstack/api/response/HostResponse.java

    }

-    public Boolean getHasEnoughCapacity() {
+    public Boolean hasEnoughCapacity() {


good choice

DaanHoogland · Apr 27, 2018

api/src/main/java/org/apache/cloudstack/api/response/ServiceOfferingResponse.java

    }

-    public Boolean getDefaultUse() {
+    public Boolean isDefaultUse() {


isForDefaultUse() ??!?

DaanHoogland · Apr 27, 2018

plugins/database/quota/src/main/java/org/apache/cloudstack/api/command/QuotaSummaryCmd.java

    }

-    public Boolean getListAll() {
+    public Boolean isListAll() {


shouldListAll()???

DaanHoogland · May 2, 2018

@blueorangutan package

blueorangutan · May 2, 2018

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

blueorangutan · May 2, 2018

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

DaanHoogland · May 3, 2018

@blueorangutan test

blueorangutan · May 3, 2018

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

blueorangutan · May 4, 2018

Trillian test result (tid-2589)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 43342 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr2600-t2589-kvm-centos7.zip
Intermitten failure detected: /marvin/tests/smoke/test_certauthority_root.py
Intermitten failure detected: /marvin/tests/smoke/test_internal_lb.py
Intermitten failure detected: /marvin/tests/smoke/test_vm_life_cycle.py
Intermitten failure detected: /marvin/tests/smoke/test_volumes.py
Intermitten failure detected: /marvin/tests/smoke/test_vpc_vpn.py
Intermitten failure detected: /marvin/tests/smoke/test_hostha_kvm.py
Smoke tests completed. 63 look OK, 4 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_04_rvpc_internallb_haproxy_stats_on_all_interfaces	`Error`	393.66	test_internal_lb.py
test_01_secured_vm_migration	`Error`	43.55	test_vm_life_cycle.py
test_02_not_secured_vm_migration	`Error`	42.51	test_vm_life_cycle.py
test_03_secured_to_nonsecured_vm_migration	`Error`	42.51	test_vm_life_cycle.py
test_04_nonsecured_to_secured_vm_migration	`Error`	42.49	test_vm_life_cycle.py
test_11_migrate_volume_and_change_offering	`Error`	129.09	test_volumes.py
test_hostha_enable_ha_when_host_in_maintenance	`Error`	3.48	test_hostha_kvm.py

DaanHoogland · May 4, 2018

these test resulta are dissappointing so i did a recursive case insensitive search of the changed method names:
for i in getOfferHa,isOfferHa,GetLimitCpuUse,isLimitCpuUse,getVolatileVm,isVolatileVm,getForVirtualNetwork,isForVirtualNetwork,getIsSourceNat,isSourceNat,getIsStaticNat,isStaticNat,getRestartRequired,isRestartRequired,getSpecifyIpRanges,isSpecifyIpRanges,getDisplayVm,isDisplayVm,getDisplayVpc,isDisplayVpc,getLocalStorageActive,isLocalStorageActive,getHasEnoughCapacity,hasEnoughCapacity,getSuitableForMigration,isSuitableForMigration,getDefaultUse,isDefaultUse,getForDisplay,isForDisplay,getDestroyed,isDestroyed,getListAll,isListAll,getSparseVolumes,isSparseVolumes,getPasswordEnabled,isPasswordEnabled,getRequiresHVM,isRequiresHVM,getIsPublic,isPublic,getFeatured,isFeatured,getIsExtractable,isExtractable,getBootable,isBootable,getSshKeyEnabled,isSshKeyEnabled
do
grep -ir $i *
done

baloujak:pr2600-t2589-kvm-centos7 dahn$ for i in getOfferHa,isOfferHa,GetLimitCpuUse,isLimitCpuUse,getVolatileVm,isVolatileVm,getForVirtualNetwork,isForVirtualNetwork,getIsSourceNat,isSourceNat,getIsStaticNat,isStaticNat,getRestartRequired,isRestartRequired,getSpecifyIpRanges,isSpecifyIpRanges,getDisplayVm,isDisplayVm,getDisplayVpc,isDisplayVpc,getLocalStorageActive,isLocalStorageActive,getHasEnoughCapacity,hasEnoughCapacity,getSuitableForMigration,isSuitableForMigration,getDefaultUse,isDefaultUse,getForDisplay,isForDisplay,getDestroyed,isDestroyed,getListAll,isListAll,getSparseVolumes,isSparseVolumes,getPasswordEnabled,isPasswordEnabled,getRequiresHVM,isRequiresHVM,getIsPublic,isPublic,getFeatured,isFeatured,getIsExtractable,isExtractable,getBootable,isBootable,getSshKeyEnabled,isSshKeyEnabled
> do
>   grep -ir $i *
> done
baloujak:pr2600-t2589-kvm-centos7 dahn$

no results are good results. I think we can go agead with this @rhtyd @rafaelweingartner ?

rafaelweingartner · May 8, 2018

@DaanHoogland so you checked it, and it seems that everything is ok? Is that it?
If that is the case, I think we proceed as we did with others. Travis and Jenkins are green.

…uds' Correção na migração de VMs com ISO anexada Closes apache#2600 See merge request scclouds/scclouds!1188

nitin-maharana mentioned this pull request Apr 24, 2018

CLOUDSTACK-10363: Change method names. #2599

Merged

Kui-Liu closed this Apr 24, 2018

Kui-Liu reopened this Apr 26, 2018

DaanHoogland reviewed Apr 27, 2018

View reviewed changes

yadvr approved these changes May 1, 2018

View reviewed changes

nitin-maharana approved these changes May 7, 2018

View reviewed changes

DaanHoogland approved these changes May 9, 2018

View reviewed changes

yadvr merged commit 951f73b into apache:master May 9, 2018

yadvr added this to the 4.12.0.0 milestone May 9, 2018

snyk-bot mentioned this pull request Jul 16, 2021

[Snyk] Upgrade rollup from 0.41.6 to 0.68.2 hixiomh/cloudstack#54

Open

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

Conversation

Kui-Liu commented Apr 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DaanHoogland left a comment

Choose a reason for hiding this comment

Uh oh!

DaanHoogland Apr 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DaanHoogland Apr 27, 2018

Choose a reason for hiding this comment

Uh oh!

DaanHoogland Apr 27, 2018

Choose a reason for hiding this comment

Uh oh!

DaanHoogland Apr 27, 2018

Choose a reason for hiding this comment

Uh oh!

DaanHoogland Apr 27, 2018

Choose a reason for hiding this comment

Uh oh!

DaanHoogland commented May 2, 2018

Uh oh!

blueorangutan commented May 2, 2018

Uh oh!

blueorangutan commented May 2, 2018

Uh oh!

DaanHoogland commented May 3, 2018

Uh oh!

blueorangutan commented May 3, 2018

Uh oh!

blueorangutan commented May 4, 2018

Uh oh!

DaanHoogland commented May 4, 2018

Uh oh!

rafaelweingartner commented May 8, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Kui-Liu commented Apr 24, 2018 •

edited

Loading

DaanHoogland Apr 27, 2018 •

edited

Loading