rafaelweingartner commented on a change in pull request #4194:
URL: https://github.com/apache/cloudstack/pull/4194#discussion_r460404337
##########
File path:
server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java
##########
@@ -3183,7 +3184,7 @@ public DiskOffering updateDiskOffering(final
UpdateDiskOfferingCmd cmd) {
throw new InvalidParameterValueException(String.format("Unable to
update disk offering: %s by id user: %s because it is not root-admin or
domain-admin", diskOfferingHandle.getUuid(), user.getUuid()));
}
- final boolean updateNeeded = name != null || displayText != null ||
sortKey != null || displayDiskOffering != null;
+ final boolean updateNeeded = name != null || displayText != null ||
sortKey != null || displayDiskOffering != null || tags != null;
Review comment:
Can you extract this `name != null || displayText != null || sortKey !=
null || displayDiskOffering != null || tags != null` to a method? Then, you can
document it (explain what it checks, and why), and after that, you can write
unit tests for all of the cases that the method must return true/false.
Moreover, instead of checking if it is different from null, you could use
`String.Utils.isNotBlank`
##########
File path:
server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java
##########
@@ -3207,30 +3208,13 @@ public DiskOffering updateDiskOffering(final
UpdateDiskOfferingCmd cmd) {
diskOffering.setDisplayOffering(displayDiskOffering);
}
- // Note: tag editing commented out for now;keeping the code intact,
- // might need to re-enable in next releases
- // if (tags != null)
- // {
- // if (tags.trim().isEmpty() && diskOfferingHandle.getTags() == null)
- // {
- // //no new tags; no existing tags
- // diskOffering.setTagsArray(csvTagsToList(null));
- // }
- // else if (!tags.trim().isEmpty() && diskOfferingHandle.getTags() !=
- // null)
- // {
- // //new tags + existing tags
- // List<String> oldTags = csvTagsToList(diskOfferingHandle.getTags());
- // List<String> newTags = csvTagsToList(tags);
- // oldTags.addAll(newTags);
- // diskOffering.setTagsArray(oldTags);
- // }
- // else if(!tags.trim().isEmpty())
- // {
- // //new tags; NO existing tags
- // diskOffering.setTagsArray(csvTagsToList(tags));
- // }
- // }
+ if (tags != null){
Review comment:
Use the String utils here, then you do not need two ifs
##########
File path:
server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java
##########
@@ -3207,30 +3208,13 @@ public DiskOffering updateDiskOffering(final
UpdateDiskOfferingCmd cmd) {
diskOffering.setDisplayOffering(displayDiskOffering);
}
- // Note: tag editing commented out for now;keeping the code intact,
- // might need to re-enable in next releases
- // if (tags != null)
- // {
- // if (tags.trim().isEmpty() && diskOfferingHandle.getTags() == null)
- // {
- // //no new tags; no existing tags
- // diskOffering.setTagsArray(csvTagsToList(null));
- // }
- // else if (!tags.trim().isEmpty() && diskOfferingHandle.getTags() !=
- // null)
- // {
- // //new tags + existing tags
- // List<String> oldTags = csvTagsToList(diskOfferingHandle.getTags());
- // List<String> newTags = csvTagsToList(tags);
- // oldTags.addAll(newTags);
- // diskOffering.setTagsArray(oldTags);
- // }
- // else if(!tags.trim().isEmpty())
- // {
- // //new tags; NO existing tags
- // diskOffering.setTagsArray(csvTagsToList(tags));
- // }
- // }
+ if (tags != null){
+ if(tags.isBlank()){
Review comment:
So, whenever updating the offering, we need to send the old(already
configured) tags? Even when we do not want to update them?
Otherwise, it seems that you would always set it to null (even if we do not
want that)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
