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.
--
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: dev-unsubscr...@knox.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
