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 2021/06/09 17:24:38 UTC
[GitHub] [cloudstack] SadiJr opened a new pull request #5094: Enable update compute offering tags fields
SadiJr opened a new pull request #5094:
URL: https://github.com/apache/cloudstack/pull/5094
### Description
This PR enables editing of tags and host tag in a compute service offering.
### 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)
- [x] Enhancement (improves an existing feature and functionality)
- [ ] Cleanup (Code refactoring and cleanup, that may add test cases)
### Feature/Enhancement Scale or Bug Severity
#### Feature/Enhancement Scale
- [ ] Major
- [x] Minor
### How Has This Been Tested?
It has been tested locally in a test lab. Also, I added unit tests to execute the code sanity of the affected methods.
1. I created a new Compute Service Offering, with tags and host tag.
2. I updated the tags and host tag using cloudmonkey.
3. I checked if the changes are correctly made, both in UI and DB.
--
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
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #5094: Enable update compute offering tags fields
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on a change in pull request #5094:
URL: https://github.com/apache/cloudstack/pull/5094#discussion_r680000061
##########
File path: server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java
##########
@@ -2783,84 +2786,140 @@ public ServiceOffering updateServiceOffering(final UpdateServiceOfferingCmd cmd)
throw new InvalidParameterValueException(String.format("Unable to update service offering: %s by id user: %s because it is not root-admin or domain-admin", offeringHandle.getUuid(), user.getUuid()));
}
- final boolean updateNeeded = name != null || displayText != null || sortKey != null;
- final boolean detailsUpdateNeeded = !filteredDomainIds.equals(existingDomainIds) || !filteredZoneIds.equals(existingZoneIds);
- if (!updateNeeded && !detailsUpdateNeeded) {
- return _serviceOfferingDao.findById(id);
+ executeServiceOfferingUpdate(displayText, id, name, sortKey, tags, hostTag);
+ updateServiceOfferingDetails(id, existingDomainIds, existingZoneIds, filteredDomainIds, filteredZoneIds);
+
+ ServiceOfferingVO offering = _serviceOfferingDao.findById(id);
+ CallContext.current().setEventDetails("Service offering id=" + offering.getId());
+ return offering;
+ }
+
+ /**
+ * Update Service Offering display text, name, sort key, tags and host tag for a specific Service Offering ID, if needed.
+ */
+ protected void executeServiceOfferingUpdate(String displayText, Long serviceOfferingId, String name, Integer sortKey, String tags, String hostTag) {
+ boolean updateNeeded = ObjectUtils.anyNotNull(name, displayText, sortKey, tags, hostTag);
+ if (!updateNeeded) {
+ s_logger.debug(String.format("No need to update service offering ID [%s] because there is no change in name, display text, " + "sort key, host tag or tags.",
+ serviceOfferingId));
+ return;
}
+ ServiceOfferingVO offering = _serviceOfferingDao.createForUpdate(serviceOfferingId);
- ServiceOfferingVO offering = _serviceOfferingDao.createForUpdate(id);
+ List<String> fields = new ArrayList<>();
if (name != null) {
+ fields.add("name: " + name);
offering.setName(name);
}
if (displayText != null) {
+ fields.add("display text: " + displayText);
offering.setDisplayText(displayText);
}
if (sortKey != null) {
+ fields.add("sort key: " + sortKey);
offering.setSortKey(sortKey);
}
- // 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() && offeringHandle.getTags() == null)
- // {
- // //no new tags; no existing tags
- // offering.setTagsArray(csvTagsToList(null));
- // }
- // else if (!tags.trim().isEmpty() && offeringHandle.getTags() != null)
- // {
- // //new tags + existing tags
- // List<String> oldTags = csvTagsToList(offeringHandle.getTags());
- // List<String> newTags = csvTagsToList(tags);
- // oldTags.addAll(newTags);
- // offering.setTagsArray(oldTags);
- // }
- // else if(!tags.trim().isEmpty())
- // {
- // //new tags; NO existing tags
- // offering.setTagsArray(csvTagsToList(tags));
- // }
- // }
-
- if (updateNeeded && !_serviceOfferingDao.update(id, offering)) {
- return null;
+ if (tags != null) {
+ fields.add("tags: " + tags);
+ offering.setTags(StringUtils.cleanupTags(tags));
}
- List<ServiceOfferingDetailsVO> detailsVO = new ArrayList<>();
- if(detailsUpdateNeeded) {
- SearchBuilder<ServiceOfferingDetailsVO> sb = _serviceOfferingDetailsDao.createSearchBuilder();
- sb.and("offeringId", sb.entity().getResourceId(), SearchCriteria.Op.EQ);
- sb.and("detailName", sb.entity().getName(), SearchCriteria.Op.EQ);
- sb.done();
- SearchCriteria<ServiceOfferingDetailsVO> sc = sb.create();
- sc.setParameters("offeringId", String.valueOf(id));
- if(!filteredDomainIds.equals(existingDomainIds)) {
- sc.setParameters("detailName", ApiConstants.DOMAIN_ID);
- _serviceOfferingDetailsDao.remove(sc);
- for (Long domainId : filteredDomainIds) {
- detailsVO.add(new ServiceOfferingDetailsVO(id, ApiConstants.DOMAIN_ID, String.valueOf(domainId), false));
- }
- }
- if(!filteredZoneIds.equals(existingZoneIds)) {
- sc.setParameters("detailName", ApiConstants.ZONE_ID);
- _serviceOfferingDetailsDao.remove(sc);
- for (Long zoneId : filteredZoneIds) {
- detailsVO.add(new ServiceOfferingDetailsVO(id, ApiConstants.ZONE_ID, String.valueOf(zoneId), false));
- }
+
+ if (hostTag != null) {
+ if (hostTag.trim().isEmpty()) {
+ fields.add("removing host tag");
+ offering.setHostTag(null);
+ } else {
+ fields.add("host tag: " + hostTag);
+ offering.setHostTag(hostTag);
}
}
- if (!detailsVO.isEmpty()) {
- for (ServiceOfferingDetailsVO detailVO : detailsVO) {
- _serviceOfferingDetailsDao.persist(detailVO);
- }
+
+ s_logger.debug(String.format("Updating service offering ID [%s] with [%s].", serviceOfferingId, String.join(", ", fields)));
+
+ if (!_serviceOfferingDao.update(serviceOfferingId, offering)) {
+ s_logger.warn(String.format("Can't update service offering ID [%s] with [%s].", serviceOfferingId, String.join(", ", fields)));
+ }
Review comment:
I think these contain the same info for the operator;
```suggestion
if (_serviceOfferingDao.update(serviceOfferingId, offering)) {
s_logger.debug(String.format("Updated service offering ID [%s] with [%s].", serviceOfferingId, String.join(", ", fields)));
} else {
s_logger.warn(String.format("Can't update service offering ID [%s] with [%s].", serviceOfferingId, String.join(", ", fields)));
}
```
##########
File path: server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java
##########
@@ -2783,84 +2786,140 @@ public ServiceOffering updateServiceOffering(final UpdateServiceOfferingCmd cmd)
throw new InvalidParameterValueException(String.format("Unable to update service offering: %s by id user: %s because it is not root-admin or domain-admin", offeringHandle.getUuid(), user.getUuid()));
}
- final boolean updateNeeded = name != null || displayText != null || sortKey != null;
- final boolean detailsUpdateNeeded = !filteredDomainIds.equals(existingDomainIds) || !filteredZoneIds.equals(existingZoneIds);
- if (!updateNeeded && !detailsUpdateNeeded) {
- return _serviceOfferingDao.findById(id);
+ executeServiceOfferingUpdate(displayText, id, name, sortKey, tags, hostTag);
+ updateServiceOfferingDetails(id, existingDomainIds, existingZoneIds, filteredDomainIds, filteredZoneIds);
+
+ ServiceOfferingVO offering = _serviceOfferingDao.findById(id);
+ CallContext.current().setEventDetails("Service offering id=" + offering.getId());
+ return offering;
+ }
+
+ /**
+ * Update Service Offering display text, name, sort key, tags and host tag for a specific Service Offering ID, if needed.
+ */
+ protected void executeServiceOfferingUpdate(String displayText, Long serviceOfferingId, String name, Integer sortKey, String tags, String hostTag) {
+ boolean updateNeeded = ObjectUtils.anyNotNull(name, displayText, sortKey, tags, hostTag);
+ if (!updateNeeded) {
+ s_logger.debug(String.format("No need to update service offering ID [%s] because there is no change in name, display text, " + "sort key, host tag or tags.",
+ serviceOfferingId));
+ return;
}
+ ServiceOfferingVO offering = _serviceOfferingDao.createForUpdate(serviceOfferingId);
- ServiceOfferingVO offering = _serviceOfferingDao.createForUpdate(id);
+ List<String> fields = new ArrayList<>();
if (name != null) {
+ fields.add("name: " + name);
offering.setName(name);
}
if (displayText != null) {
+ fields.add("display text: " + displayText);
offering.setDisplayText(displayText);
}
if (sortKey != null) {
+ fields.add("sort key: " + sortKey);
offering.setSortKey(sortKey);
}
- // 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() && offeringHandle.getTags() == null)
- // {
- // //no new tags; no existing tags
- // offering.setTagsArray(csvTagsToList(null));
- // }
- // else if (!tags.trim().isEmpty() && offeringHandle.getTags() != null)
- // {
- // //new tags + existing tags
- // List<String> oldTags = csvTagsToList(offeringHandle.getTags());
- // List<String> newTags = csvTagsToList(tags);
- // oldTags.addAll(newTags);
- // offering.setTagsArray(oldTags);
- // }
- // else if(!tags.trim().isEmpty())
- // {
- // //new tags; NO existing tags
- // offering.setTagsArray(csvTagsToList(tags));
- // }
- // }
-
- if (updateNeeded && !_serviceOfferingDao.update(id, offering)) {
- return null;
+ if (tags != null) {
+ fields.add("tags: " + tags);
+ offering.setTags(StringUtils.cleanupTags(tags));
}
- List<ServiceOfferingDetailsVO> detailsVO = new ArrayList<>();
- if(detailsUpdateNeeded) {
- SearchBuilder<ServiceOfferingDetailsVO> sb = _serviceOfferingDetailsDao.createSearchBuilder();
- sb.and("offeringId", sb.entity().getResourceId(), SearchCriteria.Op.EQ);
- sb.and("detailName", sb.entity().getName(), SearchCriteria.Op.EQ);
- sb.done();
- SearchCriteria<ServiceOfferingDetailsVO> sc = sb.create();
- sc.setParameters("offeringId", String.valueOf(id));
- if(!filteredDomainIds.equals(existingDomainIds)) {
- sc.setParameters("detailName", ApiConstants.DOMAIN_ID);
- _serviceOfferingDetailsDao.remove(sc);
- for (Long domainId : filteredDomainIds) {
- detailsVO.add(new ServiceOfferingDetailsVO(id, ApiConstants.DOMAIN_ID, String.valueOf(domainId), false));
- }
- }
- if(!filteredZoneIds.equals(existingZoneIds)) {
- sc.setParameters("detailName", ApiConstants.ZONE_ID);
- _serviceOfferingDetailsDao.remove(sc);
- for (Long zoneId : filteredZoneIds) {
- detailsVO.add(new ServiceOfferingDetailsVO(id, ApiConstants.ZONE_ID, String.valueOf(zoneId), false));
- }
+
+ if (hostTag != null) {
+ if (hostTag.trim().isEmpty()) {
+ fields.add("removing host tag");
+ offering.setHostTag(null);
+ } else {
+ fields.add("host tag: " + hostTag);
+ offering.setHostTag(hostTag);
}
}
- if (!detailsVO.isEmpty()) {
- for (ServiceOfferingDetailsVO detailVO : detailsVO) {
- _serviceOfferingDetailsDao.persist(detailVO);
- }
+
+ s_logger.debug(String.format("Updating service offering ID [%s] with [%s].", serviceOfferingId, String.join(", ", fields)));
+
+ if (!_serviceOfferingDao.update(serviceOfferingId, offering)) {
+ s_logger.warn(String.format("Can't update service offering ID [%s] with [%s].", serviceOfferingId, String.join(", ", fields)));
+ }
+ }
+
+ /**
+ * Update Service Offering Details for specific domains and zones.
+ */
+ private void updateServiceOfferingDetails(final Long serviceOfferingId, List<Long> existingDomainIds, List<Long> existingZoneIds, List<Long> filteredDomainIds,
+ List<Long> filteredZoneIds) {
+ boolean detailsUpdateNeeded = !filteredDomainIds.equals(existingDomainIds) || !filteredZoneIds.equals(existingZoneIds);
+ if (!detailsUpdateNeeded) {
+ s_logger.debug(String.format("No need to update details for service offering ID [%s] for filtered domain IDs [%s], existing domain IDs [%s], "
+ + "filtered zone IDs [%s], and existing zone IDs [%s].", serviceOfferingId, filteredDomainIds, existingDomainIds, filteredZoneIds, existingZoneIds));
+ return;
+ }
+ List<ServiceOfferingDetailsVO> detailsVO = new ArrayList<>();
Review comment:
better use plural in the name of this list to avoid confusion i.e. `detailsVOs`
##########
File path: server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java
##########
@@ -2783,84 +2786,140 @@ public ServiceOffering updateServiceOffering(final UpdateServiceOfferingCmd cmd)
throw new InvalidParameterValueException(String.format("Unable to update service offering: %s by id user: %s because it is not root-admin or domain-admin", offeringHandle.getUuid(), user.getUuid()));
}
- final boolean updateNeeded = name != null || displayText != null || sortKey != null;
- final boolean detailsUpdateNeeded = !filteredDomainIds.equals(existingDomainIds) || !filteredZoneIds.equals(existingZoneIds);
- if (!updateNeeded && !detailsUpdateNeeded) {
- return _serviceOfferingDao.findById(id);
+ executeServiceOfferingUpdate(displayText, id, name, sortKey, tags, hostTag);
+ updateServiceOfferingDetails(id, existingDomainIds, existingZoneIds, filteredDomainIds, filteredZoneIds);
+
+ ServiceOfferingVO offering = _serviceOfferingDao.findById(id);
+ CallContext.current().setEventDetails("Service offering id=" + offering.getId());
+ return offering;
+ }
+
+ /**
+ * Update Service Offering display text, name, sort key, tags and host tag for a specific Service Offering ID, if needed.
+ */
+ protected void executeServiceOfferingUpdate(String displayText, Long serviceOfferingId, String name, Integer sortKey, String tags, String hostTag) {
+ boolean updateNeeded = ObjectUtils.anyNotNull(name, displayText, sortKey, tags, hostTag);
+ if (!updateNeeded) {
+ s_logger.debug(String.format("No need to update service offering ID [%s] because there is no change in name, display text, " + "sort key, host tag or tags.",
+ serviceOfferingId));
+ return;
}
+ ServiceOfferingVO offering = _serviceOfferingDao.createForUpdate(serviceOfferingId);
- ServiceOfferingVO offering = _serviceOfferingDao.createForUpdate(id);
+ List<String> fields = new ArrayList<>();
if (name != null) {
+ fields.add("name: " + name);
offering.setName(name);
}
if (displayText != null) {
+ fields.add("display text: " + displayText);
offering.setDisplayText(displayText);
}
if (sortKey != null) {
+ fields.add("sort key: " + sortKey);
offering.setSortKey(sortKey);
}
- // 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() && offeringHandle.getTags() == null)
- // {
- // //no new tags; no existing tags
- // offering.setTagsArray(csvTagsToList(null));
- // }
- // else if (!tags.trim().isEmpty() && offeringHandle.getTags() != null)
- // {
- // //new tags + existing tags
- // List<String> oldTags = csvTagsToList(offeringHandle.getTags());
- // List<String> newTags = csvTagsToList(tags);
- // oldTags.addAll(newTags);
- // offering.setTagsArray(oldTags);
- // }
- // else if(!tags.trim().isEmpty())
- // {
- // //new tags; NO existing tags
- // offering.setTagsArray(csvTagsToList(tags));
- // }
- // }
-
- if (updateNeeded && !_serviceOfferingDao.update(id, offering)) {
- return null;
+ if (tags != null) {
+ fields.add("tags: " + tags);
+ offering.setTags(StringUtils.cleanupTags(tags));
}
- List<ServiceOfferingDetailsVO> detailsVO = new ArrayList<>();
- if(detailsUpdateNeeded) {
- SearchBuilder<ServiceOfferingDetailsVO> sb = _serviceOfferingDetailsDao.createSearchBuilder();
- sb.and("offeringId", sb.entity().getResourceId(), SearchCriteria.Op.EQ);
- sb.and("detailName", sb.entity().getName(), SearchCriteria.Op.EQ);
- sb.done();
- SearchCriteria<ServiceOfferingDetailsVO> sc = sb.create();
- sc.setParameters("offeringId", String.valueOf(id));
- if(!filteredDomainIds.equals(existingDomainIds)) {
- sc.setParameters("detailName", ApiConstants.DOMAIN_ID);
- _serviceOfferingDetailsDao.remove(sc);
- for (Long domainId : filteredDomainIds) {
- detailsVO.add(new ServiceOfferingDetailsVO(id, ApiConstants.DOMAIN_ID, String.valueOf(domainId), false));
- }
- }
- if(!filteredZoneIds.equals(existingZoneIds)) {
- sc.setParameters("detailName", ApiConstants.ZONE_ID);
- _serviceOfferingDetailsDao.remove(sc);
- for (Long zoneId : filteredZoneIds) {
- detailsVO.add(new ServiceOfferingDetailsVO(id, ApiConstants.ZONE_ID, String.valueOf(zoneId), false));
- }
+
+ if (hostTag != null) {
+ if (hostTag.trim().isEmpty()) {
+ fields.add("removing host tag");
+ offering.setHostTag(null);
+ } else {
+ fields.add("host tag: " + hostTag);
+ offering.setHostTag(hostTag);
}
}
- if (!detailsVO.isEmpty()) {
- for (ServiceOfferingDetailsVO detailVO : detailsVO) {
- _serviceOfferingDetailsDao.persist(detailVO);
- }
+
+ s_logger.debug(String.format("Updating service offering ID [%s] with [%s].", serviceOfferingId, String.join(", ", fields)));
+
+ if (!_serviceOfferingDao.update(serviceOfferingId, offering)) {
+ s_logger.warn(String.format("Can't update service offering ID [%s] with [%s].", serviceOfferingId, String.join(", ", fields)));
+ }
+ }
+
+ /**
+ * Update Service Offering Details for specific domains and zones.
+ */
+ private void updateServiceOfferingDetails(final Long serviceOfferingId, List<Long> existingDomainIds, List<Long> existingZoneIds, List<Long> filteredDomainIds,
+ List<Long> filteredZoneIds) {
+ boolean detailsUpdateNeeded = !filteredDomainIds.equals(existingDomainIds) || !filteredZoneIds.equals(existingZoneIds);
+ if (!detailsUpdateNeeded) {
+ s_logger.debug(String.format("No need to update details for service offering ID [%s] for filtered domain IDs [%s], existing domain IDs [%s], "
+ + "filtered zone IDs [%s], and existing zone IDs [%s].", serviceOfferingId, filteredDomainIds, existingDomainIds, filteredZoneIds, existingZoneIds));
+ return;
+ }
+ List<ServiceOfferingDetailsVO> detailsVO = new ArrayList<>();
+
+ removeServiceOfferingsForDomains(serviceOfferingId, existingDomainIds, filteredDomainIds, detailsVO);
+ removeServiceOfferingsForZones(serviceOfferingId, existingZoneIds, filteredZoneIds, detailsVO);
+
+ if (detailsVO.isEmpty()) {
+ s_logger.info(String.format("No need to update service offering details for service offering ID [%s], because the details remained the same.", serviceOfferingId));
+ return;
+ }
+
+ for (ServiceOfferingDetailsVO detailVO : detailsVO) {
Review comment:
here's why I want that plural. so `detailsVO` and `detailsVOs`, It is a more difference than `detailVO` and `detailsVO` .
##########
File path: server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java
##########
@@ -2783,84 +2786,140 @@ public ServiceOffering updateServiceOffering(final UpdateServiceOfferingCmd cmd)
throw new InvalidParameterValueException(String.format("Unable to update service offering: %s by id user: %s because it is not root-admin or domain-admin", offeringHandle.getUuid(), user.getUuid()));
}
- final boolean updateNeeded = name != null || displayText != null || sortKey != null;
- final boolean detailsUpdateNeeded = !filteredDomainIds.equals(existingDomainIds) || !filteredZoneIds.equals(existingZoneIds);
- if (!updateNeeded && !detailsUpdateNeeded) {
- return _serviceOfferingDao.findById(id);
+ executeServiceOfferingUpdate(displayText, id, name, sortKey, tags, hostTag);
+ updateServiceOfferingDetails(id, existingDomainIds, existingZoneIds, filteredDomainIds, filteredZoneIds);
+
+ ServiceOfferingVO offering = _serviceOfferingDao.findById(id);
+ CallContext.current().setEventDetails("Service offering id=" + offering.getId());
+ return offering;
+ }
+
+ /**
+ * Update Service Offering display text, name, sort key, tags and host tag for a specific Service Offering ID, if needed.
+ */
+ protected void executeServiceOfferingUpdate(String displayText, Long serviceOfferingId, String name, Integer sortKey, String tags, String hostTag) {
+ boolean updateNeeded = ObjectUtils.anyNotNull(name, displayText, sortKey, tags, hostTag);
+ if (!updateNeeded) {
+ s_logger.debug(String.format("No need to update service offering ID [%s] because there is no change in name, display text, " + "sort key, host tag or tags.",
+ serviceOfferingId));
+ return;
Review comment:
thanks for taking the effort to reduce this method's length at least a bit :+1:
##########
File path: server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java
##########
@@ -2783,84 +2786,140 @@ public ServiceOffering updateServiceOffering(final UpdateServiceOfferingCmd cmd)
throw new InvalidParameterValueException(String.format("Unable to update service offering: %s by id user: %s because it is not root-admin or domain-admin", offeringHandle.getUuid(), user.getUuid()));
}
- final boolean updateNeeded = name != null || displayText != null || sortKey != null;
- final boolean detailsUpdateNeeded = !filteredDomainIds.equals(existingDomainIds) || !filteredZoneIds.equals(existingZoneIds);
- if (!updateNeeded && !detailsUpdateNeeded) {
- return _serviceOfferingDao.findById(id);
+ executeServiceOfferingUpdate(displayText, id, name, sortKey, tags, hostTag);
+ updateServiceOfferingDetails(id, existingDomainIds, existingZoneIds, filteredDomainIds, filteredZoneIds);
+
+ ServiceOfferingVO offering = _serviceOfferingDao.findById(id);
+ CallContext.current().setEventDetails("Service offering id=" + offering.getId());
+ return offering;
+ }
+
+ /**
+ * Update Service Offering display text, name, sort key, tags and host tag for a specific Service Offering ID, if needed.
+ */
+ protected void executeServiceOfferingUpdate(String displayText, Long serviceOfferingId, String name, Integer sortKey, String tags, String hostTag) {
+ boolean updateNeeded = ObjectUtils.anyNotNull(name, displayText, sortKey, tags, hostTag);
+ if (!updateNeeded) {
+ s_logger.debug(String.format("No need to update service offering ID [%s] because there is no change in name, display text, " + "sort key, host tag or tags.",
+ serviceOfferingId));
+ return;
}
+ ServiceOfferingVO offering = _serviceOfferingDao.createForUpdate(serviceOfferingId);
- ServiceOfferingVO offering = _serviceOfferingDao.createForUpdate(id);
+ List<String> fields = new ArrayList<>();
if (name != null) {
+ fields.add("name: " + name);
offering.setName(name);
}
if (displayText != null) {
+ fields.add("display text: " + displayText);
offering.setDisplayText(displayText);
}
if (sortKey != null) {
+ fields.add("sort key: " + sortKey);
offering.setSortKey(sortKey);
}
- // 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() && offeringHandle.getTags() == null)
- // {
- // //no new tags; no existing tags
- // offering.setTagsArray(csvTagsToList(null));
- // }
- // else if (!tags.trim().isEmpty() && offeringHandle.getTags() != null)
- // {
- // //new tags + existing tags
- // List<String> oldTags = csvTagsToList(offeringHandle.getTags());
- // List<String> newTags = csvTagsToList(tags);
- // oldTags.addAll(newTags);
- // offering.setTagsArray(oldTags);
- // }
- // else if(!tags.trim().isEmpty())
- // {
- // //new tags; NO existing tags
- // offering.setTagsArray(csvTagsToList(tags));
- // }
- // }
-
- if (updateNeeded && !_serviceOfferingDao.update(id, offering)) {
- return null;
+ if (tags != null) {
+ fields.add("tags: " + tags);
+ offering.setTags(StringUtils.cleanupTags(tags));
}
- List<ServiceOfferingDetailsVO> detailsVO = new ArrayList<>();
- if(detailsUpdateNeeded) {
- SearchBuilder<ServiceOfferingDetailsVO> sb = _serviceOfferingDetailsDao.createSearchBuilder();
- sb.and("offeringId", sb.entity().getResourceId(), SearchCriteria.Op.EQ);
- sb.and("detailName", sb.entity().getName(), SearchCriteria.Op.EQ);
- sb.done();
- SearchCriteria<ServiceOfferingDetailsVO> sc = sb.create();
- sc.setParameters("offeringId", String.valueOf(id));
- if(!filteredDomainIds.equals(existingDomainIds)) {
- sc.setParameters("detailName", ApiConstants.DOMAIN_ID);
- _serviceOfferingDetailsDao.remove(sc);
- for (Long domainId : filteredDomainIds) {
- detailsVO.add(new ServiceOfferingDetailsVO(id, ApiConstants.DOMAIN_ID, String.valueOf(domainId), false));
- }
- }
- if(!filteredZoneIds.equals(existingZoneIds)) {
- sc.setParameters("detailName", ApiConstants.ZONE_ID);
- _serviceOfferingDetailsDao.remove(sc);
- for (Long zoneId : filteredZoneIds) {
- detailsVO.add(new ServiceOfferingDetailsVO(id, ApiConstants.ZONE_ID, String.valueOf(zoneId), false));
- }
+
+ if (hostTag != null) {
+ if (hostTag.trim().isEmpty()) {
+ fields.add("removing host tag");
+ offering.setHostTag(null);
+ } else {
+ fields.add("host tag: " + hostTag);
+ offering.setHostTag(hostTag);
}
}
- if (!detailsVO.isEmpty()) {
- for (ServiceOfferingDetailsVO detailVO : detailsVO) {
- _serviceOfferingDetailsDao.persist(detailVO);
- }
+
+ s_logger.debug(String.format("Updating service offering ID [%s] with [%s].", serviceOfferingId, String.join(", ", fields)));
+
+ if (!_serviceOfferingDao.update(serviceOfferingId, offering)) {
+ s_logger.warn(String.format("Can't update service offering ID [%s] with [%s].", serviceOfferingId, String.join(", ", fields)));
+ }
+ }
+
+ /**
+ * Update Service Offering Details for specific domains and zones.
+ */
+ private void updateServiceOfferingDetails(final Long serviceOfferingId, List<Long> existingDomainIds, List<Long> existingZoneIds, List<Long> filteredDomainIds,
+ List<Long> filteredZoneIds) {
+ boolean detailsUpdateNeeded = !filteredDomainIds.equals(existingDomainIds) || !filteredZoneIds.equals(existingZoneIds);
+ if (!detailsUpdateNeeded) {
+ s_logger.debug(String.format("No need to update details for service offering ID [%s] for filtered domain IDs [%s], existing domain IDs [%s], "
+ + "filtered zone IDs [%s], and existing zone IDs [%s].", serviceOfferingId, filteredDomainIds, existingDomainIds, filteredZoneIds, existingZoneIds));
+ return;
+ }
+ List<ServiceOfferingDetailsVO> detailsVO = new ArrayList<>();
+
+ removeServiceOfferingsForDomains(serviceOfferingId, existingDomainIds, filteredDomainIds, detailsVO);
+ removeServiceOfferingsForZones(serviceOfferingId, existingZoneIds, filteredZoneIds, detailsVO);
+
+ if (detailsVO.isEmpty()) {
+ s_logger.info(String.format("No need to update service offering details for service offering ID [%s], because the details remained the same.", serviceOfferingId));
+ return;
+ }
+
+ for (ServiceOfferingDetailsVO detailVO : detailsVO) {
+ s_logger.debug(String.format("Creating service offering details ID [%s] with resource ID [%s], name [%s] and value [%s]. ", detailVO.getId(), detailVO.getResourceId(),
+ detailVO.getName(), detailVO.getValue()));
+ _serviceOfferingDetailsDao.persist(detailVO);
+ }
+ }
+
+ private SearchCriteria<ServiceOfferingDetailsVO> createSearchCriteriaDeleteServiceOffering(final Long serviceOfferingId) {
+ SearchBuilder<ServiceOfferingDetailsVO> sb = _serviceOfferingDetailsDao.createSearchBuilder();
+ sb.and("offeringId", sb.entity().getResourceId(), SearchCriteria.Op.EQ);
+ sb.and("detailName", sb.entity().getName(), SearchCriteria.Op.EQ);
+ sb.done();
+ SearchCriteria<ServiceOfferingDetailsVO> sc = sb.create();
+ sc.setParameters("offeringId", String.valueOf(serviceOfferingId));
+ return sc;
+ }
Review comment:
this belongs in the applicable `Dao`
##########
File path: server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java
##########
@@ -2783,84 +2786,140 @@ public ServiceOffering updateServiceOffering(final UpdateServiceOfferingCmd cmd)
throw new InvalidParameterValueException(String.format("Unable to update service offering: %s by id user: %s because it is not root-admin or domain-admin", offeringHandle.getUuid(), user.getUuid()));
}
- final boolean updateNeeded = name != null || displayText != null || sortKey != null;
- final boolean detailsUpdateNeeded = !filteredDomainIds.equals(existingDomainIds) || !filteredZoneIds.equals(existingZoneIds);
- if (!updateNeeded && !detailsUpdateNeeded) {
- return _serviceOfferingDao.findById(id);
+ executeServiceOfferingUpdate(displayText, id, name, sortKey, tags, hostTag);
+ updateServiceOfferingDetails(id, existingDomainIds, existingZoneIds, filteredDomainIds, filteredZoneIds);
+
+ ServiceOfferingVO offering = _serviceOfferingDao.findById(id);
+ CallContext.current().setEventDetails("Service offering id=" + offering.getId());
+ return offering;
+ }
+
+ /**
+ * Update Service Offering display text, name, sort key, tags and host tag for a specific Service Offering ID, if needed.
+ */
+ protected void executeServiceOfferingUpdate(String displayText, Long serviceOfferingId, String name, Integer sortKey, String tags, String hostTag) {
+ boolean updateNeeded = ObjectUtils.anyNotNull(name, displayText, sortKey, tags, hostTag);
+ if (!updateNeeded) {
+ s_logger.debug(String.format("No need to update service offering ID [%s] because there is no change in name, display text, " + "sort key, host tag or tags.",
+ serviceOfferingId));
+ return;
}
+ ServiceOfferingVO offering = _serviceOfferingDao.createForUpdate(serviceOfferingId);
- ServiceOfferingVO offering = _serviceOfferingDao.createForUpdate(id);
+ List<String> fields = new ArrayList<>();
if (name != null) {
+ fields.add("name: " + name);
offering.setName(name);
}
if (displayText != null) {
+ fields.add("display text: " + displayText);
offering.setDisplayText(displayText);
}
if (sortKey != null) {
+ fields.add("sort key: " + sortKey);
offering.setSortKey(sortKey);
}
- // 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() && offeringHandle.getTags() == null)
- // {
- // //no new tags; no existing tags
- // offering.setTagsArray(csvTagsToList(null));
- // }
- // else if (!tags.trim().isEmpty() && offeringHandle.getTags() != null)
- // {
- // //new tags + existing tags
- // List<String> oldTags = csvTagsToList(offeringHandle.getTags());
- // List<String> newTags = csvTagsToList(tags);
- // oldTags.addAll(newTags);
- // offering.setTagsArray(oldTags);
- // }
- // else if(!tags.trim().isEmpty())
- // {
- // //new tags; NO existing tags
- // offering.setTagsArray(csvTagsToList(tags));
- // }
- // }
-
- if (updateNeeded && !_serviceOfferingDao.update(id, offering)) {
- return null;
+ if (tags != null) {
+ fields.add("tags: " + tags);
+ offering.setTags(StringUtils.cleanupTags(tags));
}
- List<ServiceOfferingDetailsVO> detailsVO = new ArrayList<>();
- if(detailsUpdateNeeded) {
- SearchBuilder<ServiceOfferingDetailsVO> sb = _serviceOfferingDetailsDao.createSearchBuilder();
- sb.and("offeringId", sb.entity().getResourceId(), SearchCriteria.Op.EQ);
- sb.and("detailName", sb.entity().getName(), SearchCriteria.Op.EQ);
- sb.done();
- SearchCriteria<ServiceOfferingDetailsVO> sc = sb.create();
- sc.setParameters("offeringId", String.valueOf(id));
- if(!filteredDomainIds.equals(existingDomainIds)) {
- sc.setParameters("detailName", ApiConstants.DOMAIN_ID);
- _serviceOfferingDetailsDao.remove(sc);
- for (Long domainId : filteredDomainIds) {
- detailsVO.add(new ServiceOfferingDetailsVO(id, ApiConstants.DOMAIN_ID, String.valueOf(domainId), false));
- }
- }
- if(!filteredZoneIds.equals(existingZoneIds)) {
- sc.setParameters("detailName", ApiConstants.ZONE_ID);
- _serviceOfferingDetailsDao.remove(sc);
- for (Long zoneId : filteredZoneIds) {
- detailsVO.add(new ServiceOfferingDetailsVO(id, ApiConstants.ZONE_ID, String.valueOf(zoneId), false));
- }
+
+ if (hostTag != null) {
+ if (hostTag.trim().isEmpty()) {
+ fields.add("removing host tag");
+ offering.setHostTag(null);
+ } else {
+ fields.add("host tag: " + hostTag);
+ offering.setHostTag(hostTag);
}
}
- if (!detailsVO.isEmpty()) {
- for (ServiceOfferingDetailsVO detailVO : detailsVO) {
- _serviceOfferingDetailsDao.persist(detailVO);
- }
+
+ s_logger.debug(String.format("Updating service offering ID [%s] with [%s].", serviceOfferingId, String.join(", ", fields)));
+
+ if (!_serviceOfferingDao.update(serviceOfferingId, offering)) {
+ s_logger.warn(String.format("Can't update service offering ID [%s] with [%s].", serviceOfferingId, String.join(", ", fields)));
+ }
+ }
+
+ /**
+ * Update Service Offering Details for specific domains and zones.
+ */
+ private void updateServiceOfferingDetails(final Long serviceOfferingId, List<Long> existingDomainIds, List<Long> existingZoneIds, List<Long> filteredDomainIds,
+ List<Long> filteredZoneIds) {
+ boolean detailsUpdateNeeded = !filteredDomainIds.equals(existingDomainIds) || !filteredZoneIds.equals(existingZoneIds);
+ if (!detailsUpdateNeeded) {
+ s_logger.debug(String.format("No need to update details for service offering ID [%s] for filtered domain IDs [%s], existing domain IDs [%s], "
+ + "filtered zone IDs [%s], and existing zone IDs [%s].", serviceOfferingId, filteredDomainIds, existingDomainIds, filteredZoneIds, existingZoneIds));
+ return;
+ }
+ List<ServiceOfferingDetailsVO> detailsVO = new ArrayList<>();
+
+ removeServiceOfferingsForDomains(serviceOfferingId, existingDomainIds, filteredDomainIds, detailsVO);
+ removeServiceOfferingsForZones(serviceOfferingId, existingZoneIds, filteredZoneIds, detailsVO);
+
+ if (detailsVO.isEmpty()) {
+ s_logger.info(String.format("No need to update service offering details for service offering ID [%s], because the details remained the same.", serviceOfferingId));
+ return;
+ }
Review comment:
isn't it better info to report what has changed, instead of what hasn't?
##########
File path: server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java
##########
@@ -2783,84 +2786,140 @@ public ServiceOffering updateServiceOffering(final UpdateServiceOfferingCmd cmd)
throw new InvalidParameterValueException(String.format("Unable to update service offering: %s by id user: %s because it is not root-admin or domain-admin", offeringHandle.getUuid(), user.getUuid()));
}
- final boolean updateNeeded = name != null || displayText != null || sortKey != null;
- final boolean detailsUpdateNeeded = !filteredDomainIds.equals(existingDomainIds) || !filteredZoneIds.equals(existingZoneIds);
- if (!updateNeeded && !detailsUpdateNeeded) {
- return _serviceOfferingDao.findById(id);
+ executeServiceOfferingUpdate(displayText, id, name, sortKey, tags, hostTag);
+ updateServiceOfferingDetails(id, existingDomainIds, existingZoneIds, filteredDomainIds, filteredZoneIds);
+
+ ServiceOfferingVO offering = _serviceOfferingDao.findById(id);
+ CallContext.current().setEventDetails("Service offering id=" + offering.getId());
+ return offering;
+ }
+
+ /**
+ * Update Service Offering display text, name, sort key, tags and host tag for a specific Service Offering ID, if needed.
+ */
+ protected void executeServiceOfferingUpdate(String displayText, Long serviceOfferingId, String name, Integer sortKey, String tags, String hostTag) {
+ boolean updateNeeded = ObjectUtils.anyNotNull(name, displayText, sortKey, tags, hostTag);
+ if (!updateNeeded) {
+ s_logger.debug(String.format("No need to update service offering ID [%s] because there is no change in name, display text, " + "sort key, host tag or tags.",
+ serviceOfferingId));
+ return;
}
+ ServiceOfferingVO offering = _serviceOfferingDao.createForUpdate(serviceOfferingId);
- ServiceOfferingVO offering = _serviceOfferingDao.createForUpdate(id);
+ List<String> fields = new ArrayList<>();
if (name != null) {
+ fields.add("name: " + name);
offering.setName(name);
}
if (displayText != null) {
+ fields.add("display text: " + displayText);
offering.setDisplayText(displayText);
}
if (sortKey != null) {
+ fields.add("sort key: " + sortKey);
offering.setSortKey(sortKey);
}
- // 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() && offeringHandle.getTags() == null)
- // {
- // //no new tags; no existing tags
- // offering.setTagsArray(csvTagsToList(null));
- // }
- // else if (!tags.trim().isEmpty() && offeringHandle.getTags() != null)
- // {
- // //new tags + existing tags
- // List<String> oldTags = csvTagsToList(offeringHandle.getTags());
- // List<String> newTags = csvTagsToList(tags);
- // oldTags.addAll(newTags);
- // offering.setTagsArray(oldTags);
- // }
- // else if(!tags.trim().isEmpty())
- // {
- // //new tags; NO existing tags
- // offering.setTagsArray(csvTagsToList(tags));
- // }
- // }
-
- if (updateNeeded && !_serviceOfferingDao.update(id, offering)) {
- return null;
+ if (tags != null) {
+ fields.add("tags: " + tags);
+ offering.setTags(StringUtils.cleanupTags(tags));
}
- List<ServiceOfferingDetailsVO> detailsVO = new ArrayList<>();
- if(detailsUpdateNeeded) {
- SearchBuilder<ServiceOfferingDetailsVO> sb = _serviceOfferingDetailsDao.createSearchBuilder();
- sb.and("offeringId", sb.entity().getResourceId(), SearchCriteria.Op.EQ);
- sb.and("detailName", sb.entity().getName(), SearchCriteria.Op.EQ);
- sb.done();
- SearchCriteria<ServiceOfferingDetailsVO> sc = sb.create();
- sc.setParameters("offeringId", String.valueOf(id));
- if(!filteredDomainIds.equals(existingDomainIds)) {
- sc.setParameters("detailName", ApiConstants.DOMAIN_ID);
- _serviceOfferingDetailsDao.remove(sc);
- for (Long domainId : filteredDomainIds) {
- detailsVO.add(new ServiceOfferingDetailsVO(id, ApiConstants.DOMAIN_ID, String.valueOf(domainId), false));
- }
- }
- if(!filteredZoneIds.equals(existingZoneIds)) {
- sc.setParameters("detailName", ApiConstants.ZONE_ID);
- _serviceOfferingDetailsDao.remove(sc);
- for (Long zoneId : filteredZoneIds) {
- detailsVO.add(new ServiceOfferingDetailsVO(id, ApiConstants.ZONE_ID, String.valueOf(zoneId), false));
- }
+
+ if (hostTag != null) {
+ if (hostTag.trim().isEmpty()) {
+ fields.add("removing host tag");
+ offering.setHostTag(null);
+ } else {
+ fields.add("host tag: " + hostTag);
+ offering.setHostTag(hostTag);
}
}
- if (!detailsVO.isEmpty()) {
- for (ServiceOfferingDetailsVO detailVO : detailsVO) {
- _serviceOfferingDetailsDao.persist(detailVO);
- }
+
+ s_logger.debug(String.format("Updating service offering ID [%s] with [%s].", serviceOfferingId, String.join(", ", fields)));
+
+ if (!_serviceOfferingDao.update(serviceOfferingId, offering)) {
+ s_logger.warn(String.format("Can't update service offering ID [%s] with [%s].", serviceOfferingId, String.join(", ", fields)));
+ }
+ }
+
+ /**
+ * Update Service Offering Details for specific domains and zones.
+ */
+ private void updateServiceOfferingDetails(final Long serviceOfferingId, List<Long> existingDomainIds, List<Long> existingZoneIds, List<Long> filteredDomainIds,
+ List<Long> filteredZoneIds) {
+ boolean detailsUpdateNeeded = !filteredDomainIds.equals(existingDomainIds) || !filteredZoneIds.equals(existingZoneIds);
+ if (!detailsUpdateNeeded) {
+ s_logger.debug(String.format("No need to update details for service offering ID [%s] for filtered domain IDs [%s], existing domain IDs [%s], "
+ + "filtered zone IDs [%s], and existing zone IDs [%s].", serviceOfferingId, filteredDomainIds, existingDomainIds, filteredZoneIds, existingZoneIds));
+ return;
+ }
+ List<ServiceOfferingDetailsVO> detailsVO = new ArrayList<>();
+
+ removeServiceOfferingsForDomains(serviceOfferingId, existingDomainIds, filteredDomainIds, detailsVO);
+ removeServiceOfferingsForZones(serviceOfferingId, existingZoneIds, filteredZoneIds, detailsVO);
+
+ if (detailsVO.isEmpty()) {
+ s_logger.info(String.format("No need to update service offering details for service offering ID [%s], because the details remained the same.", serviceOfferingId));
+ return;
+ }
+
+ for (ServiceOfferingDetailsVO detailVO : detailsVO) {
+ s_logger.debug(String.format("Creating service offering details ID [%s] with resource ID [%s], name [%s] and value [%s]. ", detailVO.getId(), detailVO.getResourceId(),
+ detailVO.getName(), detailVO.getValue()));
+ _serviceOfferingDetailsDao.persist(detailVO);
+ }
+ }
+
+ private SearchCriteria<ServiceOfferingDetailsVO> createSearchCriteriaDeleteServiceOffering(final Long serviceOfferingId) {
+ SearchBuilder<ServiceOfferingDetailsVO> sb = _serviceOfferingDetailsDao.createSearchBuilder();
+ sb.and("offeringId", sb.entity().getResourceId(), SearchCriteria.Op.EQ);
+ sb.and("detailName", sb.entity().getName(), SearchCriteria.Op.EQ);
+ sb.done();
+ SearchCriteria<ServiceOfferingDetailsVO> sc = sb.create();
+ sc.setParameters("offeringId", String.valueOf(serviceOfferingId));
+ return sc;
+ }
+
+ private void removeServiceOfferingsForZones(final Long serviceOfferingId, List<Long> existingZoneIds, List<Long> filteredZoneIds, List<ServiceOfferingDetailsVO> detailsVO) {
+ SearchCriteria<ServiceOfferingDetailsVO> sc = createSearchCriteriaDeleteServiceOffering(serviceOfferingId);
+
+ if (filteredZoneIds.equals(existingZoneIds)) {
+ s_logger.debug(String.format("No need to remove service offering details ID [%s] for all zones.", serviceOfferingId));
+ return;
+ }
+
+ s_logger.debug(String.format("Removing service offering details ID [%s] for all zones.", serviceOfferingId));
+ sc.setParameters("detailName", ApiConstants.ZONE_ID);
+ _serviceOfferingDetailsDao.remove(sc);
+ for (Long zoneId : filteredZoneIds) {
+ ServiceOfferingDetailsVO serviceOfferingDetailsVO = new ServiceOfferingDetailsVO(serviceOfferingId, ApiConstants.ZONE_ID, String.valueOf(zoneId), false);
+ s_logger.debug(String.format("Adding new service offering details with ID [%s], zone ID [%s], name [%s] and value [%s].", serviceOfferingDetailsVO.getId(), zoneId,
+ serviceOfferingDetailsVO.getName(), serviceOfferingDetailsVO.getValue()));
+ detailsVO.add(serviceOfferingDetailsVO);
+ }
+ }
+
+ private void removeServiceOfferingsForDomains(final Long serviceOfferingId, List<Long> existingDomainIds, List<Long> filteredDomainIds,
Review comment:
should go in the `Dao`
##########
File path: server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java
##########
@@ -2783,84 +2786,140 @@ public ServiceOffering updateServiceOffering(final UpdateServiceOfferingCmd cmd)
throw new InvalidParameterValueException(String.format("Unable to update service offering: %s by id user: %s because it is not root-admin or domain-admin", offeringHandle.getUuid(), user.getUuid()));
}
- final boolean updateNeeded = name != null || displayText != null || sortKey != null;
- final boolean detailsUpdateNeeded = !filteredDomainIds.equals(existingDomainIds) || !filteredZoneIds.equals(existingZoneIds);
- if (!updateNeeded && !detailsUpdateNeeded) {
- return _serviceOfferingDao.findById(id);
+ executeServiceOfferingUpdate(displayText, id, name, sortKey, tags, hostTag);
+ updateServiceOfferingDetails(id, existingDomainIds, existingZoneIds, filteredDomainIds, filteredZoneIds);
+
+ ServiceOfferingVO offering = _serviceOfferingDao.findById(id);
+ CallContext.current().setEventDetails("Service offering id=" + offering.getId());
+ return offering;
+ }
+
+ /**
+ * Update Service Offering display text, name, sort key, tags and host tag for a specific Service Offering ID, if needed.
+ */
+ protected void executeServiceOfferingUpdate(String displayText, Long serviceOfferingId, String name, Integer sortKey, String tags, String hostTag) {
+ boolean updateNeeded = ObjectUtils.anyNotNull(name, displayText, sortKey, tags, hostTag);
+ if (!updateNeeded) {
+ s_logger.debug(String.format("No need to update service offering ID [%s] because there is no change in name, display text, " + "sort key, host tag or tags.",
+ serviceOfferingId));
+ return;
}
+ ServiceOfferingVO offering = _serviceOfferingDao.createForUpdate(serviceOfferingId);
- ServiceOfferingVO offering = _serviceOfferingDao.createForUpdate(id);
+ List<String> fields = new ArrayList<>();
if (name != null) {
+ fields.add("name: " + name);
offering.setName(name);
}
if (displayText != null) {
+ fields.add("display text: " + displayText);
offering.setDisplayText(displayText);
}
if (sortKey != null) {
+ fields.add("sort key: " + sortKey);
offering.setSortKey(sortKey);
}
- // 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() && offeringHandle.getTags() == null)
- // {
- // //no new tags; no existing tags
- // offering.setTagsArray(csvTagsToList(null));
- // }
- // else if (!tags.trim().isEmpty() && offeringHandle.getTags() != null)
- // {
- // //new tags + existing tags
- // List<String> oldTags = csvTagsToList(offeringHandle.getTags());
- // List<String> newTags = csvTagsToList(tags);
- // oldTags.addAll(newTags);
- // offering.setTagsArray(oldTags);
- // }
- // else if(!tags.trim().isEmpty())
- // {
- // //new tags; NO existing tags
- // offering.setTagsArray(csvTagsToList(tags));
- // }
- // }
-
- if (updateNeeded && !_serviceOfferingDao.update(id, offering)) {
- return null;
+ if (tags != null) {
+ fields.add("tags: " + tags);
+ offering.setTags(StringUtils.cleanupTags(tags));
}
- List<ServiceOfferingDetailsVO> detailsVO = new ArrayList<>();
- if(detailsUpdateNeeded) {
- SearchBuilder<ServiceOfferingDetailsVO> sb = _serviceOfferingDetailsDao.createSearchBuilder();
- sb.and("offeringId", sb.entity().getResourceId(), SearchCriteria.Op.EQ);
- sb.and("detailName", sb.entity().getName(), SearchCriteria.Op.EQ);
- sb.done();
- SearchCriteria<ServiceOfferingDetailsVO> sc = sb.create();
- sc.setParameters("offeringId", String.valueOf(id));
- if(!filteredDomainIds.equals(existingDomainIds)) {
- sc.setParameters("detailName", ApiConstants.DOMAIN_ID);
- _serviceOfferingDetailsDao.remove(sc);
- for (Long domainId : filteredDomainIds) {
- detailsVO.add(new ServiceOfferingDetailsVO(id, ApiConstants.DOMAIN_ID, String.valueOf(domainId), false));
- }
- }
- if(!filteredZoneIds.equals(existingZoneIds)) {
- sc.setParameters("detailName", ApiConstants.ZONE_ID);
- _serviceOfferingDetailsDao.remove(sc);
- for (Long zoneId : filteredZoneIds) {
- detailsVO.add(new ServiceOfferingDetailsVO(id, ApiConstants.ZONE_ID, String.valueOf(zoneId), false));
- }
+
+ if (hostTag != null) {
+ if (hostTag.trim().isEmpty()) {
+ fields.add("removing host tag");
+ offering.setHostTag(null);
+ } else {
+ fields.add("host tag: " + hostTag);
+ offering.setHostTag(hostTag);
}
}
- if (!detailsVO.isEmpty()) {
- for (ServiceOfferingDetailsVO detailVO : detailsVO) {
- _serviceOfferingDetailsDao.persist(detailVO);
- }
+
+ s_logger.debug(String.format("Updating service offering ID [%s] with [%s].", serviceOfferingId, String.join(", ", fields)));
+
+ if (!_serviceOfferingDao.update(serviceOfferingId, offering)) {
+ s_logger.warn(String.format("Can't update service offering ID [%s] with [%s].", serviceOfferingId, String.join(", ", fields)));
+ }
+ }
+
+ /**
+ * Update Service Offering Details for specific domains and zones.
+ */
+ private void updateServiceOfferingDetails(final Long serviceOfferingId, List<Long> existingDomainIds, List<Long> existingZoneIds, List<Long> filteredDomainIds,
+ List<Long> filteredZoneIds) {
+ boolean detailsUpdateNeeded = !filteredDomainIds.equals(existingDomainIds) || !filteredZoneIds.equals(existingZoneIds);
+ if (!detailsUpdateNeeded) {
+ s_logger.debug(String.format("No need to update details for service offering ID [%s] for filtered domain IDs [%s], existing domain IDs [%s], "
+ + "filtered zone IDs [%s], and existing zone IDs [%s].", serviceOfferingId, filteredDomainIds, existingDomainIds, filteredZoneIds, existingZoneIds));
+ return;
+ }
+ List<ServiceOfferingDetailsVO> detailsVO = new ArrayList<>();
+
+ removeServiceOfferingsForDomains(serviceOfferingId, existingDomainIds, filteredDomainIds, detailsVO);
+ removeServiceOfferingsForZones(serviceOfferingId, existingZoneIds, filteredZoneIds, detailsVO);
+
+ if (detailsVO.isEmpty()) {
+ s_logger.info(String.format("No need to update service offering details for service offering ID [%s], because the details remained the same.", serviceOfferingId));
+ return;
+ }
+
+ for (ServiceOfferingDetailsVO detailVO : detailsVO) {
+ s_logger.debug(String.format("Creating service offering details ID [%s] with resource ID [%s], name [%s] and value [%s]. ", detailVO.getId(), detailVO.getResourceId(),
+ detailVO.getName(), detailVO.getValue()));
+ _serviceOfferingDetailsDao.persist(detailVO);
+ }
+ }
+
+ private SearchCriteria<ServiceOfferingDetailsVO> createSearchCriteriaDeleteServiceOffering(final Long serviceOfferingId) {
+ SearchBuilder<ServiceOfferingDetailsVO> sb = _serviceOfferingDetailsDao.createSearchBuilder();
+ sb.and("offeringId", sb.entity().getResourceId(), SearchCriteria.Op.EQ);
+ sb.and("detailName", sb.entity().getName(), SearchCriteria.Op.EQ);
+ sb.done();
+ SearchCriteria<ServiceOfferingDetailsVO> sc = sb.create();
+ sc.setParameters("offeringId", String.valueOf(serviceOfferingId));
+ return sc;
+ }
+
+ private void removeServiceOfferingsForZones(final Long serviceOfferingId, List<Long> existingZoneIds, List<Long> filteredZoneIds, List<ServiceOfferingDetailsVO> detailsVO) {
Review comment:
should go in the `Dao`
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #5094: Enable update compute offering tags fields
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5094:
URL: https://github.com/apache/cloudstack/pull/5094#issuecomment-889639405
@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #5094: Enable update compute offering tags fields
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5094:
URL: https://github.com/apache/cloudstack/pull/5094#issuecomment-889649526
Packaging result: :heavy_check_mark: el7 :heavy_check_mark: el8 :heavy_check_mark: debian. SL-JID 704
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] SadiJr commented on pull request #5094: Enable update compute offering tags fields
Posted by GitBox <gi...@apache.org>.
SadiJr commented on pull request #5094:
URL: https://github.com/apache/cloudstack/pull/5094#issuecomment-858044230
@shwstppr, yes, is similar. I only discovered this PR after creating this one. I intend to keep this open until one of the two is accepted.
--
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
[GitHub] [cloudstack] SadiJr closed pull request #5094: Enable update compute offering tags fields
Posted by GitBox <gi...@apache.org>.
SadiJr closed pull request #5094:
URL: https://github.com/apache/cloudstack/pull/5094
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rhtyd commented on pull request #5094: Enable update compute offering tags fields
Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #5094:
URL: https://github.com/apache/cloudstack/pull/5094#issuecomment-889639254
@blueorangutan package
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] shwstppr commented on pull request #5094: Enable update compute offering tags fields
Posted by GitBox <gi...@apache.org>.
shwstppr commented on pull request #5094:
URL: https://github.com/apache/cloudstack/pull/5094#issuecomment-857896884
@SadiJr is this similar https://github.com/apache/cloudstack/pull/5043?
--
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
[GitHub] [cloudstack] SadiJr commented on pull request #5094: Enable update compute offering tags fields
Posted by GitBox <gi...@apache.org>.
SadiJr commented on pull request #5094:
URL: https://github.com/apache/cloudstack/pull/5094#issuecomment-891117133
@DaanHoogland I appreciate the review and apologize for the delay in replying. I saw that the PR #5043 was merged, so I'll close this one.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org