nvazquez commented on code in PR #7397: URL: https://github.com/apache/cloudstack/pull/7397#discussion_r1177202111
########## api/src/main/java/org/apache/cloudstack/api/command/user/vm/CreateVMScheduleCmd.java: ########## @@ -0,0 +1,150 @@ +/* + * 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.user.vm; + +import com.cloud.exception.InvalidParameterValueException; +import com.cloud.vm.VirtualMachine; +import org.apache.cloudstack.api.APICommand; +import org.apache.cloudstack.api.ApiConstants; +import org.apache.cloudstack.api.BaseCmd; +import org.apache.cloudstack.api.Parameter; +import org.apache.cloudstack.api.response.UserVmResponse; +import org.apache.cloudstack.api.response.VMScheduleResponse; +import org.apache.cloudstack.vm.schedule.VMScheduleManager; + +import javax.inject.Inject; +import java.util.Date; + +@APICommand(name = "createVMSchedule", description = "Create VM Schedule", responseObject = VMScheduleResponse.class, + requestHasSensitiveInfo = false, responseHasSensitiveInfo = false, since = "4.19.0") +public class CreateVMScheduleCmd extends BaseCmd { + + @Inject + VMScheduleManager vmScheduleManager; + + @Parameter(name = ApiConstants.VIRTUAL_MACHINE_ID, + type = CommandType.UUID, + entityType = UserVmResponse.class, + required = true, + description = "ID of the VM for which schedule is to be defined") + private Long vmId; + + @Parameter(name = ApiConstants.DESCRIPTION, + type = CommandType.STRING, + required = false, + description = "Description of the schedule") + private String description; + + @Parameter(name = ApiConstants.SCHEDULE, + type = CommandType.STRING, + required = true, + description = "Schedule for action on VM in cron format") Review Comment: Can we add an input example on the description? ########## api/src/main/java/org/apache/cloudstack/vm/schedule/VMScheduleManager.java: ########## @@ -0,0 +1,40 @@ +/* + * 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.vm.schedule; + +import org.apache.cloudstack.api.command.user.vm.CreateVMScheduleCmd; +import org.apache.cloudstack.api.command.user.vm.DeleteVMScheduleCmd; +import org.apache.cloudstack.api.command.user.vm.ListVMScheduleCmd; +import org.apache.cloudstack.api.command.user.vm.UpdateVMScheduleCmd; +import org.apache.cloudstack.api.response.ListResponse; +import org.apache.cloudstack.api.response.VMScheduleResponse; + +public interface VMScheduleManager { + VMScheduleResponse createSchedule(CreateVMScheduleCmd createVMScheduleCmd); + + VMScheduleResponse createResponse(VMSchedule vmSchedule); + + ListResponse<VMScheduleResponse> listSchedule(ListVMScheduleCmd listVMScheduleCmd); + + VMScheduleResponse updateSchedule(UpdateVMScheduleCmd updateVMScheduleCmd); + + long removeScheduleByVmId(long vmId, boolean expunge); + + Long removeSchedule(DeleteVMScheduleCmd deleteVMScheduleCmd); Review Comment: Can we refactor to have only one interface method for removing a schedule but internally discriminating if its only by VM id or more params from the command? ########## engine/schema/src/main/java/org/apache/cloudstack/vm/schedule/dao/VMScheduleDaoImpl.java: ########## @@ -0,0 +1,100 @@ +/* + * 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.vm.schedule.dao; + +import com.cloud.utils.Pair; +import com.cloud.utils.db.Filter; +import com.cloud.utils.db.GenericDaoBase; +import com.cloud.utils.db.SearchBuilder; +import com.cloud.utils.db.SearchCriteria; +import org.apache.cloudstack.api.ApiConstants; +import org.apache.cloudstack.vm.schedule.VMSchedule; +import org.apache.cloudstack.vm.schedule.VMScheduleVO; +import org.springframework.stereotype.Component; + +import java.util.Date; +import java.util.List; + +@Component +public class VMScheduleDaoImpl extends GenericDaoBase<VMScheduleVO, Long> implements VMScheduleDao { + + @Override + public List<VMScheduleVO> listAllActiveSchedules() { + // WHERE enabled = true AND (end_date IS NULL OR end_date > current_date) + SearchBuilder<VMScheduleVO> sb = createSearchBuilder(); + sb.and(ApiConstants.ENABLED, sb.entity().getEnabled(), SearchCriteria.Op.EQ); Review Comment: We can also define the search criterias as class fields and using them on the dao methods by only setting the parameters ########## engine/schema/src/main/java/org/apache/cloudstack/vm/schedule/VMScheduledJobVO.java: ########## @@ -0,0 +1,107 @@ +/* + * 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.vm.schedule; + +import javax.persistence.Column; +import javax.persistence.Entity; +import javax.persistence.EnumType; +import javax.persistence.Enumerated; +import javax.persistence.GeneratedValue; +import javax.persistence.GenerationType; +import javax.persistence.Id; +import javax.persistence.Table; +import javax.persistence.Temporal; +import javax.persistence.TemporalType; +import java.util.Date; +import java.util.UUID; + +@Entity +@Table(name = "vm_scheduled_job") +public class VMScheduledJobVO implements VMScheduledJob { + @Id + @GeneratedValue(strategy = GenerationType.IDENTITY) + @Column(name = "id") + Long id; + @Column(name = "uuid", nullable = false) + String uuid = UUID.randomUUID().toString(); Review Comment: Either this initialization or the one in the constructor are redundant ########## api/src/test/java/org/apache/cloudstack/api/command/user/vm/CreateVMScheduleCmdTest.java: ########## @@ -0,0 +1,97 @@ +/* + * 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.user.vm; + +import com.cloud.exception.InvalidParameterValueException; +import com.cloud.utils.db.EntityManager; +import com.cloud.vm.VirtualMachine; +import org.apache.cloudstack.api.response.VMScheduleResponse; +import org.apache.cloudstack.vm.schedule.VMScheduleManager; +import org.junit.Assert; +import org.junit.Before; +import org.junit.Test; +import org.mockito.InjectMocks; +import org.mockito.Mock; +import org.mockito.Mockito; +import org.mockito.MockitoAnnotations; + +import java.security.InvalidParameterException; + +public class CreateVMScheduleCmdTest { + @Mock + public VMScheduleManager vmScheduleManager; + @Mock + public EntityManager entityManager; + @InjectMocks + private CreateVMScheduleCmd createVMScheduleCmd = new CreateVMScheduleCmd(); + + @Before + public void setUp() throws Exception { + MockitoAnnotations.initMocks(this); + } + + /** + * given: "We have a VMScheduleManager and CreateVMScheduleCmd" + * when: "CreateVMScheduleCmd is executed successfully" + * then: "a VMSchedule response is created" + */ + @Test + public void successfulExecution() { Review Comment: Minor refactor: can the these methods names start with the 'test' prefix? -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
