You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by GitBox <gi...@apache.org> on 2020/07/25 13:22:22 UTC
[GitHub] [cloudstack] rafaelweingartner commented on a change in pull request #4194: enable update tags on disk offerings
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:
users@infra.apache.org