vishesh92 commented on code in PR #10773: URL: https://github.com/apache/cloudstack/pull/10773#discussion_r2072863861
########## api/src/main/java/org/apache/cloudstack/api/command/admin/guest/DeleteGuestOsCategoryCmd.java: ########## @@ -0,0 +1,78 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package org.apache.cloudstack.api.command.admin.guest; + +import org.apache.cloudstack.acl.RoleType; +import org.apache.cloudstack.api.APICommand; +import org.apache.cloudstack.api.ApiConstants; +import org.apache.cloudstack.api.ApiErrorCode; +import org.apache.cloudstack.api.BaseCmd; +import org.apache.cloudstack.api.Parameter; +import org.apache.cloudstack.api.ServerApiException; +import org.apache.cloudstack.api.response.GuestOSCategoryResponse; +import org.apache.cloudstack.api.response.SuccessResponse; + +import com.cloud.user.Account; + + +@APICommand(name = "deleteOsCategory", + description = "Deletes an OS category", + responseObject = SuccessResponse.class, + requestHasSensitiveInfo = false, + responseHasSensitiveInfo = false, + since = "4.20.1", + authorized = {RoleType.Admin}) +public class DeleteGuestOsCategoryCmd extends BaseCmd { + + + ///////////////////////////////////////////////////// + //////////////// API parameters ///////////////////// + ///////////////////////////////////////////////////// + + @Parameter(name = ApiConstants.ID, type = CommandType.UUID, entityType = GuestOSCategoryResponse.class, + required = true, description = "ID of the guest OS") Review Comment: ```suggestion required = true, description = "ID of the guest OS category") ``` ########## api/src/main/java/org/apache/cloudstack/api/command/admin/guest/DeleteGuestOsCategoryCmd.java: ########## @@ -0,0 +1,78 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package org.apache.cloudstack.api.command.admin.guest; + +import org.apache.cloudstack.acl.RoleType; +import org.apache.cloudstack.api.APICommand; +import org.apache.cloudstack.api.ApiConstants; +import org.apache.cloudstack.api.ApiErrorCode; +import org.apache.cloudstack.api.BaseCmd; +import org.apache.cloudstack.api.Parameter; +import org.apache.cloudstack.api.ServerApiException; +import org.apache.cloudstack.api.response.GuestOSCategoryResponse; +import org.apache.cloudstack.api.response.SuccessResponse; + +import com.cloud.user.Account; + + +@APICommand(name = "deleteOsCategory", + description = "Deletes an OS category", + responseObject = SuccessResponse.class, + requestHasSensitiveInfo = false, + responseHasSensitiveInfo = false, + since = "4.20.1", + authorized = {RoleType.Admin}) +public class DeleteGuestOsCategoryCmd extends BaseCmd { + + + ///////////////////////////////////////////////////// + //////////////// API parameters ///////////////////// + ///////////////////////////////////////////////////// + + @Parameter(name = ApiConstants.ID, type = CommandType.UUID, entityType = GuestOSCategoryResponse.class, + required = true, description = "ID of the guest OS") + private Long id; + + ///////////////////////////////////////////////////// + /////////////////// Accessors /////////////////////// + ///////////////////////////////////////////////////// + + public Long getId() { + return id; + } + + ///////////////////////////////////////////////////// + /////////////// API Implementation/////////////////// + ///////////////////////////////////////////////////// + + @Override + public void execute() { + boolean result = _mgr.deleteGuestOsCategory(this); + if (result) { + SuccessResponse response = new SuccessResponse(getCommandName()); + setResponseObject(response); + } else { + throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR, "Failed to remove guest OS"); Review Comment: ```suggestion throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR, "Failed to remove guest OS category"); ``` ########## api/src/main/java/org/apache/cloudstack/api/command/admin/guest/UpdateGuestOsCategoryCmd.java: ########## @@ -0,0 +1,102 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package org.apache.cloudstack.api.command.admin.guest; + +import org.apache.cloudstack.acl.RoleType; +import org.apache.cloudstack.api.APICommand; +import org.apache.cloudstack.api.ApiConstants; +import org.apache.cloudstack.api.ApiErrorCode; +import org.apache.cloudstack.api.BaseCmd; +import org.apache.cloudstack.api.Parameter; +import org.apache.cloudstack.api.ServerApiException; +import org.apache.cloudstack.api.response.GuestOSCategoryResponse; + +import com.cloud.storage.GuestOsCategory; +import com.cloud.user.Account; + +@APICommand(name = "updateOsCategory", + description = "Updates an OS category", + responseObject = GuestOSCategoryResponse.class, + requestHasSensitiveInfo = false, + responseHasSensitiveInfo = false, + since = "4.20.1", + authorized = {RoleType.Admin}) +public class UpdateGuestOsCategoryCmd extends BaseCmd { + + + + ///////////////////////////////////////////////////// + //////////////// API parameters ///////////////////// + ///////////////////////////////////////////////////// + + @Parameter(name = ApiConstants.ID, type = CommandType.UUID, entityType = GuestOSCategoryResponse.class, + required = true, description = "ID of the OS Category") + private Long id; + + @Parameter(name = ApiConstants.NAME, type = CommandType.STRING, description = "Name for the OS category") + private String name; + + @Parameter(name = ApiConstants.IS_FEATURED, type = CommandType.BOOLEAN, + description = "Whether the category is featured or not") + private Boolean featured; + + @Parameter(name = ApiConstants.SORT_KEY, type = CommandType.INTEGER, + description = "sort key of the OS category for listing") + private Integer sortKey; + + ///////////////////////////////////////////////////// + /////////////////// Accessors /////////////////////// + ///////////////////////////////////////////////////// + + public Long getId() { + return id; + } + + public String getName() { + return name; + } + + public Boolean isFeatured() { + return featured; + } + + public Integer getSortKey() { + return sortKey; + } + + ///////////////////////////////////////////////////// + /////////////// API Implementation/////////////////// + ///////////////////////////////////////////////////// + + @Override + public long getEntityOwnerId() { + return Account.ACCOUNT_ID_SYSTEM; + } + + @Override + public void execute() { + GuestOsCategory guestOs = _mgr.updateGuestOsCategory(this); + if (guestOs != null) { + GuestOSCategoryResponse response = _responseGenerator.createGuestOSCategoryResponse(guestOs); + response.setResponseName(getCommandName()); + setResponseObject(response); + } else { + throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR, "Failed to update guest OS type"); Review Comment: ```suggestion throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR, "Failed to update guest OS category"); ``` ########## api/src/main/java/org/apache/cloudstack/api/command/admin/guest/UpdateGuestOsCmd.java: ########## @@ -71,7 +77,7 @@ public String getOsDisplayName() { return osDisplayName; } - public Map getDetails() { + public Map<String, String> getDetails() { Map<String, String> detailsMap = new HashMap<>();; if (MapUtils.isNotEmpty(detailsMap)) { Collection<?> servicesCollection = details.values(); Review Comment: ```suggestion public Map<String, String> getDetails() { Map<String, String> detailsMap = new HashMap<>(); if (MapUtils.isNotEmpty(details)) { Collection<?> servicesCollection = details.values(); ``` From the code, I don't think this is even working. ########## server/src/main/java/com/cloud/server/ManagementServerImpl.java: ########## @@ -2723,29 +2728,109 @@ public Pair<List<? extends GuestOS>, Integer> listGuestOSByCriteria(final ListGu @Override public Pair<List<? extends GuestOsCategory>, Integer> listGuestOSCategoriesByCriteria(final ListGuestOsCategoriesCmd cmd) { - final Filter searchFilter = new Filter(GuestOSCategoryVO.class, "id", true, cmd.getStartIndex(), cmd.getPageSizeVal()); + final Filter searchFilter = new Filter(GuestOSCategoryVO.class, "sortKey", true, + cmd.getStartIndex(), cmd.getPageSizeVal()); + searchFilter.addOrderBy(GuestOSCategoryVO.class, "id", true); final Long id = cmd.getId(); final String name = cmd.getName(); final String keyword = cmd.getKeyword(); + final Boolean featured = cmd.isFeatured(); + final Boolean isIso = cmd.isIso(); + final Boolean isVnf = cmd.isVnf(); + final Long zoneId = cmd.getZoneId(); + final CPU.CPUArch arch = cmd.getArch(); - final SearchCriteria<GuestOSCategoryVO> sc = _guestOSCategoryDao.createSearchCriteria(); - + final SearchBuilder<GuestOSCategoryVO> sb = _guestOSCategoryDao.createSearchBuilder(); + sb.and("id", sb.entity().getId(), SearchCriteria.Op.EQ); + sb.and("name", sb.entity().getId(), SearchCriteria.Op.LIKE); + sb.and("keyword", sb.entity().getId(), SearchCriteria.Op.LIKE); Review Comment: ```suggestion sb.and("name", sb.entity().getName(), SearchCriteria.Op.LIKE); sb.and("keyword", sb.entity().getName(), SearchCriteria.Op.LIKE); ``` ########## api/src/main/java/org/apache/cloudstack/api/command/admin/guest/UpdateGuestOsCmd.java: ########## @@ -50,7 +50,7 @@ public class UpdateGuestOsCmd extends BaseAsyncCmd { @Parameter(name = ApiConstants.ID, type = CommandType.UUID, entityType = GuestOSResponse.class, required = true, description = "UUID of the Guest OS") private Long id; - @Parameter(name = ApiConstants.OS_DISPLAY_NAME, type = CommandType.STRING, required = true, description = "Unique display name for Guest OS") Review Comment: JFYI, this will require a change in go sdk [here](https://github.com/apache/cloudstack-go/blob/main/generate/requiredParams.go) to ensure the SDK is backward compatible. ########## engine/schema/src/main/java/com/cloud/storage/dao/GuestOSCategoryDaoImpl.java: ########## @@ -27,7 +27,7 @@ public class GuestOSCategoryDaoImpl extends GenericDaoBase<GuestOSCategoryVO, Long> implements GuestOSCategoryDao { protected GuestOSCategoryDaoImpl() { - + this._count = "select count(distinct id) from guest_os_category WHERE "; Review Comment: Why is this required? ########## server/src/test/java/com/cloud/server/ManagementServerImplTest.java: ########## @@ -124,27 +139,27 @@ public class ManagementServerImplTest { UserDataManager userDataManager; @Spy + @InjectMocks ManagementServerImpl spy = new ManagementServerImpl(); @Mock UserVmDetailsDao userVmDetailsDao; @Mock HostDetailsDao hostDetailsDao; + + @Mock + GuestOSCategoryDao guestOSCategoryDao; + + @Mock + GuestOSDao guestOSDao; Review Comment: ```suggestion GuestOSCategoryDao _guestOSCategoryDao; @Mock GuestOSDao _guestOSDao; ``` Use the same name for dao as in the class being tested. -- 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-unsubscr...@cloudstack.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
