Phillippko commented on code in PR #7740: URL: https://github.com/apache/ignite-3/pull/7740#discussion_r2917631316
########## modules/cli/src/main/java/org/apache/ignite/internal/cli/commands/cluster/ClusterNameMixin.java: ########## @@ -0,0 +1,83 @@ +/* + * 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.ignite.internal.cli.commands.cluster; + +import static org.apache.ignite.internal.cli.commands.Options.Constants.CLUSTER_NAME_OPTION; +import static org.apache.ignite.internal.cli.commands.Options.Constants.CLUSTER_NAME_OPTION_DESC; + +import java.util.Arrays; +import java.util.stream.Collectors; +import org.apache.ignite.internal.cli.util.ConfigurationArgsParseException; +import org.jetbrains.annotations.Nullable; +import picocli.CommandLine.Option; +import picocli.CommandLine.Parameters; + +/** + * Mixin class for cluster name option in REPL mode. Review Comment: ```suggestion * Mixin class for cluster name. ``` ########## modules/cli/src/integrationTest/java/org/apache/ignite/internal/cli/call/configuration/ItRenameCallTest.java: ########## @@ -0,0 +1,68 @@ +/* + * 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.ignite.internal.cli.call.configuration; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; + +import jakarta.inject.Inject; +import org.apache.ignite.internal.cli.CliIntegrationTest; +import org.apache.ignite.internal.cli.call.cluster.status.ClusterRenameCall; +import org.apache.ignite.internal.cli.call.cluster.status.ClusterRenameCallInput; +import org.apache.ignite.internal.cli.call.cluster.status.ClusterStatusCall; +import org.apache.ignite.internal.cli.core.call.DefaultCallOutput; +import org.apache.ignite.internal.cli.core.call.UrlCallInput; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; + +/** + * Tests for {@link ClusterRenameCall}. + */ +public class ItRenameCallTest extends CliIntegrationTest { Review Comment: ```suggestion public class ItClusterRenameCallTest extends CliIntegrationTest { ``` ########## modules/cli/src/test/java/org/apache/ignite/internal/cli/commands/cluster/RenameTest.java: ########## @@ -0,0 +1,108 @@ +/* + * 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.ignite.internal.cli.commands.cluster; + +import static org.junit.jupiter.api.Assertions.assertAll; +import static org.junit.jupiter.params.provider.Arguments.arguments; + +import java.util.function.Function; +import java.util.stream.Stream; +import org.apache.ignite.internal.cli.call.cluster.status.ClusterRenameCall; +import org.apache.ignite.internal.cli.call.cluster.status.ClusterRenameCallInput; +import org.apache.ignite.internal.cli.commands.CliCommandTestBase; +import org.apache.ignite.internal.cli.commands.TopLevelCliCommand; +import org.apache.ignite.internal.cli.core.call.Call; +import org.apache.ignite.internal.cli.core.call.CallInput; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; + +class RenameTest extends CliCommandTestBase { Review Comment: ```suggestion class ClusterRenameCommandTest extends CliCommandTestBase { ``` ########## modules/cli/src/test/java/org/apache/ignite/internal/cli/commands/cluster/RenameTest.java: ########## @@ -0,0 +1,108 @@ +/* + * 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.ignite.internal.cli.commands.cluster; + +import static org.junit.jupiter.api.Assertions.assertAll; +import static org.junit.jupiter.params.provider.Arguments.arguments; + +import java.util.function.Function; +import java.util.stream.Stream; +import org.apache.ignite.internal.cli.call.cluster.status.ClusterRenameCall; +import org.apache.ignite.internal.cli.call.cluster.status.ClusterRenameCallInput; +import org.apache.ignite.internal.cli.commands.CliCommandTestBase; +import org.apache.ignite.internal.cli.commands.TopLevelCliCommand; +import org.apache.ignite.internal.cli.core.call.Call; +import org.apache.ignite.internal.cli.core.call.CallInput; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; + +class RenameTest extends CliCommandTestBase { + @Override + protected Class<?> getCommandClass() { + return TopLevelCliCommand.class; + } + + private static Stream<Arguments> calls() { + return Stream.of( + arguments( + "cluster rename", + ClusterRenameCall.class, + ClusterRenameCallInput.class, + (Function<ClusterRenameCallInput, String>) ClusterRenameCallInput::getName + ) + ); + } + + @ParameterizedTest + @MethodSource("calls") + void noParameter(String command) { + // When executed without arguments + execute(command); + + assertAll( + () -> assertExitCodeIs(2), + this::assertOutputIsEmpty, + () -> assertErrOutputContains("Failed to parse name.") + ); + } + + @ParameterizedTest + @MethodSource("calls") + <IT extends CallInput, OT, T extends Call<IT, OT>> void unquotedParameter( + String command, + Class<T> callClass, + Class<IT> callInputClass, + Function<IT, String> nameFunction + ) { + checkParameters(command, callClass, callInputClass, nameFunction, "test", "test"); + } + + @ParameterizedTest + @MethodSource("calls") + <IT extends CallInput, OT, T extends Call<IT, OT>> void quotedParameter( + String command, + Class<T> callClass, + Class<IT> callInputClass, + Function<IT, String> nameFunction + ) { + checkParameters(command, callClass, callInputClass, nameFunction, "\"test\"", "test"); + } + + @ParameterizedTest + @MethodSource("calls") + <IT extends CallInput, OT, T extends Call<IT, OT>> void unquotedParameters( + String command, + Class<T> callClass, + Class<IT> callInputClass, + Function<IT, String> nameFunction + ) { + checkParameters(command, callClass, callInputClass, nameFunction, "test1 test2", "test1 test2"); Review Comment: let's use parameterizedtest to unite these tests, passing args / expected name ########## modules/cli/src/integrationTest/java/org/apache/ignite/internal/cli/call/configuration/ItRenameCallTest.java: ########## @@ -0,0 +1,68 @@ +/* + * 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.ignite.internal.cli.call.configuration; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; + +import jakarta.inject.Inject; +import org.apache.ignite.internal.cli.CliIntegrationTest; +import org.apache.ignite.internal.cli.call.cluster.status.ClusterRenameCall; +import org.apache.ignite.internal.cli.call.cluster.status.ClusterRenameCallInput; +import org.apache.ignite.internal.cli.call.cluster.status.ClusterStatusCall; +import org.apache.ignite.internal.cli.core.call.DefaultCallOutput; +import org.apache.ignite.internal.cli.core.call.UrlCallInput; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; + +/** + * Tests for {@link ClusterRenameCall}. + */ +public class ItRenameCallTest extends CliIntegrationTest { Review Comment: Discussed with Vadim Pakhnushev, there is no point in separate tests for calls. Please replace with a test for command ALso we need tests for different cases. Quoted, unquoted, empty name, etc ########## modules/cli/src/main/java/org/apache/ignite/internal/cli/call/cluster/status/ClusterRenameCall.java: ########## @@ -0,0 +1,60 @@ +/* + * 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.ignite.internal.cli.call.cluster.status; + +import jakarta.inject.Singleton; +import org.apache.ignite.internal.cli.core.call.Call; +import org.apache.ignite.internal.cli.core.call.DefaultCallOutput; +import org.apache.ignite.internal.cli.core.exception.IgniteCliApiException; +import org.apache.ignite.internal.cli.core.rest.ApiClientFactory; +import org.apache.ignite.rest.client.api.ClusterManagementApi; +import org.apache.ignite.rest.client.invoker.ApiException; + +/** + * Renames the cluster. + */ +@Singleton +public class ClusterRenameCall implements Call<ClusterRenameCallInput, String> { + private final ApiClientFactory clientFactory; + + public ClusterRenameCall(ApiClientFactory clientFactory) { + this.clientFactory = clientFactory; + } + + /** {@inheritDoc} */ + @Override + public DefaultCallOutput<String> execute(ClusterRenameCallInput input) { + ClusterManagementApi client = createApiClient(input); + + try { + return updateClusterConfig(client, input); + } catch (ApiException | IllegalArgumentException e) { Review Comment: ```suggestion } catch (ApiException) { ``` don't see where would illegal argument come from ########## modules/cli/src/main/java/org/apache/ignite/internal/cli/call/cluster/status/ClusterRenameCall.java: ########## @@ -0,0 +1,60 @@ +/* + * 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.ignite.internal.cli.call.cluster.status; + +import jakarta.inject.Singleton; +import org.apache.ignite.internal.cli.core.call.Call; +import org.apache.ignite.internal.cli.core.call.DefaultCallOutput; +import org.apache.ignite.internal.cli.core.exception.IgniteCliApiException; +import org.apache.ignite.internal.cli.core.rest.ApiClientFactory; +import org.apache.ignite.rest.client.api.ClusterManagementApi; +import org.apache.ignite.rest.client.invoker.ApiException; + +/** + * Renames the cluster. + */ +@Singleton +public class ClusterRenameCall implements Call<ClusterRenameCallInput, String> { + private final ApiClientFactory clientFactory; + + public ClusterRenameCall(ApiClientFactory clientFactory) { + this.clientFactory = clientFactory; + } + + /** {@inheritDoc} */ + @Override + public DefaultCallOutput<String> execute(ClusterRenameCallInput input) { + ClusterManagementApi client = createApiClient(input); + + try { + return updateClusterConfig(client, input); + } catch (ApiException | IllegalArgumentException e) { + return DefaultCallOutput.failure(new IgniteCliApiException(e, input.getClusterUrl())); + } + } + + private DefaultCallOutput<String> updateClusterConfig(ClusterManagementApi api, ClusterRenameCallInput input) Review Comment: I would inline these methods ########## modules/cli/src/main/java/org/apache/ignite/internal/cli/commands/cluster/ClusterNameMixin.java: ########## @@ -0,0 +1,83 @@ +/* + * 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.ignite.internal.cli.commands.cluster; + +import static org.apache.ignite.internal.cli.commands.Options.Constants.CLUSTER_NAME_OPTION; +import static org.apache.ignite.internal.cli.commands.Options.Constants.CLUSTER_NAME_OPTION_DESC; + +import java.util.Arrays; +import java.util.stream.Collectors; +import org.apache.ignite.internal.cli.util.ConfigurationArgsParseException; +import org.jetbrains.annotations.Nullable; +import picocli.CommandLine.Option; +import picocli.CommandLine.Parameters; + +/** + * Mixin class for cluster name option in REPL mode. + */ +public class ClusterNameMixin { + @Parameters(arity = "0..1") + private String[] args; + + /** Cluster name option. */ + @Option( + names = CLUSTER_NAME_OPTION, + description = CLUSTER_NAME_OPTION_DESC + ) + private String name; + + private String nameFromArgs() { Review Comment: I wonder if we really need to provide both options (--name / just as an arg) ########## modules/cli/src/main/java/org/apache/ignite/internal/cli/commands/cluster/ClusterRenameCommand.java: ########## @@ -0,0 +1,60 @@ +/* + * 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.ignite.internal.cli.commands.cluster; + +import jakarta.inject.Inject; +import java.util.concurrent.Callable; +import org.apache.ignite.internal.cli.call.cluster.status.ClusterRenameCall; +import org.apache.ignite.internal.cli.call.cluster.status.ClusterRenameCallInput; +import org.apache.ignite.internal.cli.commands.BaseCommand; +import org.apache.ignite.internal.cli.core.call.CallExecutionPipeline; +import picocli.CommandLine.Command; +import picocli.CommandLine.Mixin; + +/** + * Command that renames the cluster. + */ +@Command(name = "rename", description = "Renames the cluster") +public class ClusterRenameCommand extends BaseCommand implements Callable<Integer> { + /** Cluster endpoint URL option. */ + @Mixin + private ClusterUrlMixin clusterUrl; + + /** Name that will be updated. */ + @Mixin + private ClusterNameMixin nameFromArgs; Review Comment: ```suggestion private ClusterNameMixin name; ``` ########## modules/cli/src/test/java/org/apache/ignite/internal/cli/commands/cluster/RenameTest.java: ########## @@ -0,0 +1,108 @@ +/* + * 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.ignite.internal.cli.commands.cluster; + +import static org.junit.jupiter.api.Assertions.assertAll; +import static org.junit.jupiter.params.provider.Arguments.arguments; + +import java.util.function.Function; +import java.util.stream.Stream; +import org.apache.ignite.internal.cli.call.cluster.status.ClusterRenameCall; +import org.apache.ignite.internal.cli.call.cluster.status.ClusterRenameCallInput; +import org.apache.ignite.internal.cli.commands.CliCommandTestBase; +import org.apache.ignite.internal.cli.commands.TopLevelCliCommand; +import org.apache.ignite.internal.cli.core.call.Call; +import org.apache.ignite.internal.cli.core.call.CallInput; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; + +class RenameTest extends CliCommandTestBase { + @Override + protected Class<?> getCommandClass() { + return TopLevelCliCommand.class; + } + + private static Stream<Arguments> calls() { + return Stream.of( + arguments( Review Comment: why one case? -- 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]
