Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add cli commons to factor out common parsing code #7301

Merged
merged 2 commits into from
Oct 30, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions airbyte-commons-cli/build.gradle
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
plugins {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it overkill to add a README for this module describing it's purpose?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lol. i guess if you have to ask it's not overkill :D I'll add it.

id "java-library"
}

dependencies {
implementation 'commons-cli:commons-cli:1.4'
}
89 changes: 89 additions & 0 deletions airbyte-commons-cli/src/main/java/io/airbyte/commons/cli/Clis.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
/*
* Copyright (c) 2021 Airbyte, Inc., all rights reserved.
*/

package io.airbyte.commons.cli;

import java.util.ArrayList;
import java.util.List;
import org.apache.commons.cli.CommandLine;
import org.apache.commons.cli.CommandLineParser;
import org.apache.commons.cli.DefaultParser;
import org.apache.commons.cli.HelpFormatter;
import org.apache.commons.cli.Option;
import org.apache.commons.cli.OptionGroup;
import org.apache.commons.cli.Options;
import org.apache.commons.cli.ParseException;

public class Clis {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should probably have at least one test for each public method in this class


/**
* Parse an options object
*
* @param args - command line args
* @param options - expected options
* @return object with parsed values.
*/
public static CommandLine parse(final String[] args, final Options options, final CommandLineParser parser, final String commandLineSyntax) {
final HelpFormatter helpFormatter = new HelpFormatter();

try {
return parser.parse(options, args);
} catch (final ParseException e) {
helpFormatter.printHelp(commandLineSyntax, options);
throw new IllegalArgumentException(e);
}
}

public static CommandLine parse(final String[] args, final Options options, final String commandLineSyntax) {
return parse(args, options, new DefaultParser(), commandLineSyntax);
}

public static CommandLine parse(final String[] args, final Options options, final CommandLineParser parser) {
return parse(args, options, parser, "");
}

public static CommandLine parse(final String[] args, final Options options) {
return parse(args, options, new DefaultParser());
}

/**
* Provide a fluent interface for building an OptionsGroup.
*
* @param isRequired - is the option group required
* @param options - options in the option group
* @return the created option group.
*/
public static OptionGroup createOptionGroup(final boolean isRequired, final Option... options) {
final OptionGroup optionGroup = new OptionGroup();
optionGroup.setRequired(isRequired);
for (final Option option : options) {
optionGroup.addOption(option);
}
return optionGroup;
}

public static CommandLineParser getRelaxedParser() {
return new RelaxedParser();
}

// https://stackoverflow.com/questions/33874902/apache-commons-cli-1-3-1-how-to-ignore-unknown-arguments
private static class RelaxedParser extends DefaultParser {

@Override
public CommandLine parse(final Options options, final String[] arguments) throws ParseException {
final List<String> knownArgs = new ArrayList<>();
for (int i = 0; i < arguments.length; i++) {
if (options.hasOption(arguments[i])) {
knownArgs.add(arguments[i]);
if (i + 1 < arguments.length && options.getOption(arguments[i]).hasArg()) {
knownArgs.add(arguments[i + 1]);
}
}
}
return super.parse(options, knownArgs.toArray(new String[0]));
}

}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
/*
* Copyright (c) 2021 Airbyte, Inc., all rights reserved.
*/

package io.airbyte.commons.cli;

import static org.junit.jupiter.api.Assertions.assertEquals;

import org.junit.jupiter.api.Test;

class ClisTest {

@Test
void testGetTaggedImageName() {
final String repository = "airbyte/repo";
final String tag = "12.3";
assertEquals("airbyte/repo:12.3", Clis.getTaggedImageName(repository, tag));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where is this method defined? am I being thick? it's not in Clis.java

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not thick. i fix.

}

}
6 changes: 4 additions & 2 deletions airbyte-integrations/bases/base-java/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,10 @@ plugins {
}

dependencies {
implementation project(':airbyte-protocol:models')
implementation project(':airbyte-commons-cli')
implementation project(':airbyte-json-validation')

implementation 'commons-cli:commons-cli:1.4'
implementation 'org.apache.sshd:sshd-mina:2.7.0'
// bouncycastle is pinned to version-match the transitive dependency from kubernetes client-java
Expand All @@ -12,8 +16,6 @@ dependencies {
implementation 'org.bouncycastle:bcpkix-jdk15on:1.66'
implementation 'org.bouncycastle:bctls-jdk15on:1.66'

implementation project(':airbyte-protocol:models')
implementation project(":airbyte-json-validation")
implementation "org.testcontainers:testcontainers:1.15.3"
implementation "org.testcontainers:jdbc:1.15.3"

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,19 +5,14 @@
package io.airbyte.integrations.base;

import com.google.common.base.Preconditions;
import io.airbyte.commons.cli.Clis;
import java.nio.file.Path;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import org.apache.commons.cli.CommandLine;
import org.apache.commons.cli.CommandLineParser;
import org.apache.commons.cli.DefaultParser;
import org.apache.commons.cli.HelpFormatter;
import org.apache.commons.cli.Option;
import org.apache.commons.cli.OptionGroup;
import org.apache.commons.cli.Options;
import org.apache.commons.cli.ParseException;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

Expand All @@ -30,52 +25,40 @@ public class IntegrationCliParser {

private static final Logger LOGGER = LoggerFactory.getLogger(IntegrationCliParser.class);

private static final OptionGroup COMMAND_GROUP = new OptionGroup();

static {
COMMAND_GROUP.setRequired(true);
COMMAND_GROUP.addOption(Option.builder()
.longOpt(Command.SPEC.toString().toLowerCase())
.desc("outputs the json configuration specification")
.build());
COMMAND_GROUP.addOption(Option.builder()
.longOpt(Command.CHECK.toString().toLowerCase())
.desc("checks the config can be used to connect")
.build());
COMMAND_GROUP.addOption(Option.builder()
.longOpt(Command.DISCOVER.toString().toLowerCase())
.desc("outputs a catalog describing the source's catalog")
.build());
COMMAND_GROUP.addOption(Option.builder()
.longOpt(Command.READ.toString().toLowerCase())
.desc("reads the source and outputs messages to STDOUT")
.build());
COMMAND_GROUP.addOption(Option.builder()
.longOpt(Command.WRITE.toString().toLowerCase())
.desc("writes messages from STDIN to the integration")
.build());
}
private static final OptionGroup COMMAND_GROUP = Clis.createOptionGroup(
true,
Option.builder()
.longOpt(Command.SPEC.toString().toLowerCase())
.desc("outputs the json configuration specification")
.build(),
Option.builder()
.longOpt(Command.CHECK.toString().toLowerCase())
.desc("checks the config can be used to connect")
.build(),
Option.builder()
.longOpt(Command.DISCOVER.toString().toLowerCase())
.desc("outputs a catalog describing the source's catalog")
.build(),
Option.builder()
.longOpt(Command.READ.toString().toLowerCase())
.desc("reads the source and outputs messages to STDOUT")
.build(),
Option.builder()
.longOpt(Command.WRITE.toString().toLowerCase())
.desc("writes messages from STDIN to the integration")
.build());

public IntegrationConfig parse(final String[] args) {
final Command command = parseCommand(args);
return parseOptions(args, command);
}

private static Command parseCommand(final String[] args) {
final CommandLineParser parser = new RelaxedParser();
final HelpFormatter helpFormatter = new HelpFormatter();

final Options options = new Options();
options.addOptionGroup(COMMAND_GROUP);

try {
final CommandLine parsed = parser.parse(options, args);
return Command.valueOf(parsed.getOptions()[0].getLongOpt().toUpperCase());
// if discover, then validate, etc...
} catch (final ParseException e) {
helpFormatter.printHelp("java-base", options);
throw new IllegalArgumentException(e);
}
final CommandLine parsed = Clis.parse(args, options, Clis.getRelaxedParser());
return Command.valueOf(parsed.getOptions()[0].getLongOpt().toUpperCase());
}

private static IntegrationConfig parseOptions(final String[] args, final Command command) {
Expand All @@ -87,26 +70,46 @@ private static IntegrationConfig parseOptions(final String[] args, final Command
case SPEC -> {
// no args.
}
case CHECK, DISCOVER -> options.addOption(Option
.builder().longOpt(JavaBaseConstants.ARGS_CONFIG_KEY).desc(JavaBaseConstants.ARGS_CONFIG_DESC).hasArg(true).required(true).build());
case CHECK, DISCOVER -> options.addOption(Option.builder()
.longOpt(JavaBaseConstants.ARGS_CONFIG_KEY)
.desc(JavaBaseConstants.ARGS_CONFIG_DESC)
.hasArg(true)
.required(true)
.build());
case READ -> {
options.addOption(Option
.builder().longOpt(JavaBaseConstants.ARGS_CONFIG_KEY).desc(JavaBaseConstants.ARGS_CONFIG_DESC).hasArg(true).required(true).build());
options.addOption(Option
.builder().longOpt(JavaBaseConstants.ARGS_CATALOG_KEY).desc(JavaBaseConstants.ARGS_CATALOG_DESC).hasArg(true).build());
options.addOption(Option
.builder().longOpt(JavaBaseConstants.ARGS_STATE_KEY).desc(JavaBaseConstants.ARGS_PATH_DESC).hasArg(true).build());
options.addOption(Option.builder()
.longOpt(JavaBaseConstants.ARGS_CONFIG_KEY)
.desc(JavaBaseConstants.ARGS_CONFIG_DESC)
.hasArg(true)
.required(true)
.build());
options.addOption(Option.builder()
.longOpt(JavaBaseConstants.ARGS_CATALOG_KEY)
.desc(JavaBaseConstants.ARGS_CATALOG_DESC)
.hasArg(true)
.build());
options.addOption(Option.builder()
.longOpt(JavaBaseConstants.ARGS_STATE_KEY)
.desc(JavaBaseConstants.ARGS_PATH_DESC)
.hasArg(true)
.build());
}
case WRITE -> {
options.addOption(Option
.builder().longOpt(JavaBaseConstants.ARGS_CONFIG_KEY).desc(JavaBaseConstants.ARGS_CONFIG_DESC).hasArg(true).required(true).build());
options.addOption(Option
.builder().longOpt(JavaBaseConstants.ARGS_CATALOG_KEY).desc(JavaBaseConstants.ARGS_CATALOG_DESC).hasArg(true).build());
options.addOption(Option.builder()
.longOpt(JavaBaseConstants.ARGS_CONFIG_KEY)
.desc(JavaBaseConstants.ARGS_CONFIG_DESC)
.hasArg(true)
.required(true).build());
options.addOption(Option.builder()
.longOpt(JavaBaseConstants.ARGS_CATALOG_KEY)
.desc(JavaBaseConstants.ARGS_CATALOG_DESC)
.hasArg(true)
.build());
}
default -> throw new IllegalStateException("Unexpected value: " + command);
}

final CommandLine parsed = runParse(options, args, command);
final CommandLine parsed = Clis.parse(args, options, command.toString().toLowerCase());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the command enum is not used here and we use string instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's the trade off of DRYing this is that the common parse function can't really reasonably know about the enum, right?

Preconditions.checkNotNull(parsed);
final Map<String, String> argsMap = new HashMap<>();
for (final Option option : parsed.getOptions()) {
Expand Down Expand Up @@ -139,35 +142,4 @@ private static IntegrationConfig parseOptions(final String[] args, final Command
}
}

private static CommandLine runParse(final Options options, final String[] args, final Command command) {
final CommandLineParser parser = new DefaultParser();
final HelpFormatter helpFormatter = new HelpFormatter();

try {
return parser.parse(options, args);
} catch (final ParseException e) {
helpFormatter.printHelp(command.toString().toLowerCase(), options);
throw new IllegalArgumentException(e);
}
}

// https://stackoverflow.com/questions/33874902/apache-commons-cli-1-3-1-how-to-ignore-unknown-arguments
private static class RelaxedParser extends DefaultParser {

@Override
public CommandLine parse(final Options options, final String[] arguments) throws ParseException {
final List<String> knownArgs = new ArrayList<>();
for (int i = 0; i < arguments.length; i++) {
if (options.hasOption(arguments[i])) {
knownArgs.add(arguments[i]);
if (i + 1 < arguments.length && options.getOption(arguments[i]).hasArg()) {
knownArgs.add(arguments[i + 1]);
}
}
}
return super.parse(options, knownArgs.toArray(new String[0]));
}

}

}
1 change: 1 addition & 0 deletions settings.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ if(!System.getenv().containsKey("SUB_BUILD")) {
include ':airbyte-api'
include ':airbyte-commons'
include ':airbyte-commons-docker'
include ':airbyte-commons-cli'
include ':airbyte-config:models' // reused by acceptance tests in connector base.
include ':airbyte-db:lib' // reused by acceptance tests in connector base.
include ':airbyte-json-validation'
Expand Down