[
https://issues.apache.org/jira/browse/KNOX-3002?focusedWorklogId=901874&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-901874
]
ASF GitHub Bot logged work on KNOX-3002:
----------------------------------------
Author: ASF GitHub Bot
Created on: 26/Jan/24 08:10
Start Date: 26/Jan/24 08:10
Worklog Time Spent: 10m
Work Description: smolnar82 commented on code in PR #835:
URL: https://github.com/apache/knox/pull/835#discussion_r1467326277
##########
gateway-server/src/test/java/org/apache/knox/gateway/util/DescriptorGeneratorTest.java:
##########
@@ -0,0 +1,55 @@
+package org.apache.knox.gateway.util;
+
+import static org.junit.Assert.assertEquals;
+
+import java.io.File;
+import java.nio.charset.Charset;
+import java.util.Arrays;
+import java.util.List;
+
+import com.fasterxml.jackson.databind.ObjectMapper;
+import org.apache.commons.io.FileUtils;
+import org.apache.commons.io.FilenameUtils;
+import org.apache.knox.gateway.model.DescriptorConfiguration;
+import org.apache.knox.gateway.model.Topology;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.TemporaryFolder;
+
+public class DescriptorGeneratorTest {
+ private static final ObjectMapper mapper = new ObjectMapper();
+ private static final String TEST_DESC_1 = "test_desc1.json";
+ private static final String TEST_PROV_1 = "test_prov1.json";
+ private static final String IMPALA_UI = "IMPALAUI";
+ private static final List<String> URLS =
+ Arrays.asList("http://amagyar-1.test.site:25000/";,
"http://amagyar-2.test.site:25000";);
+ @Rule
+ public TemporaryFolder folder= new TemporaryFolder();
Review Comment:
Instead of making this a temporary file, we could create a static file in
`src/test/resources` and have the code read that file. Since this is not a
dynamic one, I think it would make more sense and we would avoid creating a
temporary folder every time this test is running (the read I/O operation is
unavoidable, but creation can be).
##########
gateway-server/src/main/java/org/apache/knox/gateway/util/KnoxCLI.java:
##########
@@ -2392,6 +2413,55 @@ public String getUsage() {
}
+ public class GenerateDescriptorCommand extends Command {
+
+ public static final String USAGE =
+ "generate-descriptor --service-urls-file \"path/to/urls.txt\"
--service-name SERVICE_NAME \n" +
+ "--provider-name my-provider.json --descriptor-name
my-descriptor.json \n" +
+ "[--output-dir /path/to/output_dir] \n" +
+ "[--force] \n";
+ public static final String DESC =
+ "Create Knox topology descriptor file for one service\n"
+ + "Options are as follows: \n"
+ + "--service-urls-file (required) path to a text file
containing service urls \n"
+ + "--service-name (required) the name of the service, such
as WEBHDFS, IMPALAUI or HIVE \n"
+ + "--descriptor-name (required) name of descriptor to be
created \n"
+ + "--provider-name (required) name of the referenced
shared provider \n"
+ + "--output-dir (optional) output directory to save the
descriptor file \n"
+ + "--force (optional) force rewriting of existing files,
if not used, command will fail when the configs files with same name already
exist. \n";
+
+ @Override
+ public void execute() throws Exception {
+ if (StringUtils.isBlank(FilenameUtils.getExtension(providerName))
+ ||
StringUtils.isBlank(FilenameUtils.getExtension(descriptorName))) {
+ throw new IllegalArgumentException("JSON extension is required for
provider and descriptor file names");
+ }
+ if (StringUtils.isBlank(urlsFilePath) ) {
+ throw new IllegalArgumentException("Missing --service-urls-file");
+ }
+ if (!new File(urlsFilePath).isFile()) {
+ throw new IllegalArgumentException(urlsFilePath + " does not exist");
+ }
+ if (StringUtils.isBlank(serviceName)) {
+ throw new IllegalArgumentException("Missing --service-name");
+ }
Review Comment:
All of these verifications could be extracted to a private method for a
better reading experience.
##########
gateway-server/src/test/java/org/apache/knox/gateway/util/DescriptorGeneratorTest.java:
##########
@@ -0,0 +1,55 @@
+package org.apache.knox.gateway.util;
Review Comment:
Missing header? The build should fail anyway...
##########
gateway-server/src/main/java/org/apache/knox/gateway/util/DescriptorGenerator.java:
##########
@@ -0,0 +1,53 @@
+package org.apache.knox.gateway.util;
Review Comment:
Missing header? The build should fail anyway...
##########
gateway-server/src/main/java/org/apache/knox/gateway/util/ServiceUrls.java:
##########
@@ -0,0 +1,36 @@
+package org.apache.knox.gateway.util;
Review Comment:
Missing header? The build should fail anyway...
##########
gateway-server/src/main/java/org/apache/knox/gateway/util/KnoxCLI.java:
##########
@@ -2392,6 +2413,55 @@ public String getUsage() {
}
+ public class GenerateDescriptorCommand extends Command {
+
+ public static final String USAGE =
+ "generate-descriptor --service-urls-file \"path/to/urls.txt\"
--service-name SERVICE_NAME \n" +
+ "--provider-name my-provider.json --descriptor-name
my-descriptor.json \n" +
+ "[--output-dir /path/to/output_dir] \n" +
+ "[--force] \n";
+ public static final String DESC =
+ "Create Knox topology descriptor file for one service\n"
+ + "Options are as follows: \n"
+ + "--service-urls-file (required) path to a text file
containing service urls \n"
+ + "--service-name (required) the name of the service, such
as WEBHDFS, IMPALAUI or HIVE \n"
+ + "--descriptor-name (required) name of descriptor to be
created \n"
+ + "--provider-name (required) name of the referenced
shared provider \n"
+ + "--output-dir (optional) output directory to save the
descriptor file \n"
Review Comment:
This is an option field. The default value is missing from the description.
##########
gateway-server/src/main/java/org/apache/knox/gateway/util/ServiceUrls.java:
##########
@@ -0,0 +1,36 @@
+package org.apache.knox.gateway.util;
+
+import java.io.File;
+import java.io.IOException;
+import java.io.UncheckedIOException;
+import java.nio.charset.Charset;
+import java.util.Collections;
+import java.util.List;
+import java.util.stream.Collectors;
+
+import org.apache.commons.io.FileUtils;
+import org.apache.commons.lang3.StringUtils;
+
+public class ServiceUrls {
+ private final List<String> urls;
+
+ public static ServiceUrls fromFile(File urlsFilePath) {
Review Comment:
Maybe another static method that takes a `String` of `urlsFilePath` and
calls the existing one as you do in KnoxCLI?
```
public static ServiceUrls fromFile(String urlsFilePath) {
return fromFile(new File(urlsFilePath));
}
```
##########
gateway-server/src/main/java/org/apache/knox/gateway/util/KnoxCLI.java:
##########
@@ -2392,6 +2413,55 @@ public String getUsage() {
}
+ public class GenerateDescriptorCommand extends Command {
+
+ public static final String USAGE =
+ "generate-descriptor --service-urls-file \"path/to/urls.txt\"
--service-name SERVICE_NAME \n" +
+ "--provider-name my-provider.json --descriptor-name
my-descriptor.json \n" +
+ "[--output-dir /path/to/output_dir] \n" +
+ "[--force] \n";
+ public static final String DESC =
+ "Create Knox topology descriptor file for one service\n"
+ + "Options are as follows: \n"
+ + "--service-urls-file (required) path to a text file
containing service urls \n"
+ + "--service-name (required) the name of the service, such
as WEBHDFS, IMPALAUI or HIVE \n"
+ + "--descriptor-name (required) name of descriptor to be
created \n"
+ + "--provider-name (required) name of the referenced
shared provider \n"
+ + "--output-dir (optional) output directory to save the
descriptor file \n"
+ + "--force (optional) force rewriting of existing files,
if not used, command will fail when the configs files with same name already
exist. \n";
+
+ @Override
+ public void execute() throws Exception {
+ if (StringUtils.isBlank(FilenameUtils.getExtension(providerName))
+ ||
StringUtils.isBlank(FilenameUtils.getExtension(descriptorName))) {
+ throw new IllegalArgumentException("JSON extension is required for
provider and descriptor file names");
+ }
+ if (StringUtils.isBlank(urlsFilePath) ) {
+ throw new IllegalArgumentException("Missing --service-urls-file");
+ }
+ if (!new File(urlsFilePath).isFile()) {
+ throw new IllegalArgumentException(urlsFilePath + " does not exist");
+ }
+ if (StringUtils.isBlank(serviceName)) {
+ throw new IllegalArgumentException("Missing --service-name");
+ }
+ File outputDir = StringUtils.isBlank(KnoxCLI.this.outputDir) ? new
File(".") : new File(KnoxCLI.this.outputDir);
Review Comment:
Do we really need `KnoxCLI` in `KnoxCLI.this.outputDir`? I think
`this.outputDir` (or even `outputDir` only) should be enough.
##########
gateway-server/src/main/java/org/apache/knox/gateway/util/KnoxCLI.java:
##########
@@ -2392,6 +2413,55 @@ public String getUsage() {
}
+ public class GenerateDescriptorCommand extends Command {
+
+ public static final String USAGE =
+ "generate-descriptor --service-urls-file \"path/to/urls.txt\"
--service-name SERVICE_NAME \n" +
+ "--provider-name my-provider.json --descriptor-name
my-descriptor.json \n" +
+ "[--output-dir /path/to/output_dir] \n" +
+ "[--force] \n";
+ public static final String DESC =
+ "Create Knox topology descriptor file for one service\n"
+ + "Options are as follows: \n"
+ + "--service-urls-file (required) path to a text file
containing service urls \n"
+ + "--service-name (required) the name of the service, such
as WEBHDFS, IMPALAUI or HIVE \n"
+ + "--descriptor-name (required) name of descriptor to be
created \n"
+ + "--provider-name (required) name of the referenced
shared provider \n"
+ + "--output-dir (optional) output directory to save the
descriptor file \n"
+ + "--force (optional) force rewriting of existing files,
if not used, command will fail when the configs files with same name already
exist. \n";
+
+ @Override
+ public void execute() throws Exception {
+ if (StringUtils.isBlank(FilenameUtils.getExtension(providerName))
+ ||
StringUtils.isBlank(FilenameUtils.getExtension(descriptorName))) {
+ throw new IllegalArgumentException("JSON extension is required for
provider and descriptor file names");
+ }
+ if (StringUtils.isBlank(urlsFilePath) ) {
+ throw new IllegalArgumentException("Missing --service-urls-file");
+ }
+ if (!new File(urlsFilePath).isFile()) {
+ throw new IllegalArgumentException(urlsFilePath + " does not exist");
+ }
+ if (StringUtils.isBlank(serviceName)) {
+ throw new IllegalArgumentException("Missing --service-name");
+ }
+ File outputDir = StringUtils.isBlank(KnoxCLI.this.outputDir) ? new
File(".") : new File(KnoxCLI.this.outputDir);
+
+ DescriptorGenerator generator =
+ new DescriptorGenerator(descriptorName, providerName,
serviceName, ServiceUrls.fromFile(new File(urlsFilePath)));
+ generator.saveDescriptor(
Review Comment:
I think this could fit into 1 line after formatting.
Issue Time Tracking
-------------------
Worklog Id: (was: 901874)
Time Spent: 20m (was: 10m)
> KnoxCLI command for generating descriptor for a role type from a list of hosts
> ------------------------------------------------------------------------------
>
> Key: KNOX-3002
> URL: https://issues.apache.org/jira/browse/KNOX-3002
> Project: Apache Knox
> Issue Type: New Feature
> Components: KnoxCLI
> Reporter: Attila Magyar
> Assignee: Attila Magyar
> Priority: Major
> Time Spent: 20m
> Remaining Estimate: 0h
>
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
