justinmclean commented on code in PR #5641:
URL: https://github.com/apache/gravitino/pull/5641#discussion_r1851281492
##########
build.gradle.kts:
##########
@@ -294,8 +294,8 @@ subprojects {
"-Xlint:fallthrough",
"-Xlint:finally",
"-Xlint:overrides",
- "-Xlint:static",
- "-Werror"
+ "-Xlint:static"
+// "-Werror"
Review Comment:
This needs to be left in as we want warnings to show as errors.
##########
clients/cli/src/main/java/org/apache/gravitino/cli/GravitinoOptions.java:
##########
@@ -113,6 +113,7 @@ public Option createSimpleOption(String shortName, String
longName, String descr
* @return The Option object.
*/
public Option createArgOption(String shortName, String longName, String
description) {
- return new Option(shortName, longName, true, description);
+ // Support multiple arguments
Review Comment:
No real need for this comment and this alters more than tags, please just
modify tags.
##########
clients/cli/src/main/java/org/apache/gravitino/cli/commands/DeleteTag.java:
##########
@@ -38,42 +41,45 @@ public class DeleteTag extends Command {
* @param ignoreVersions If true don't check the client/server versions
match.
* @param force Force operation.
* @param metalake The name of the metalake.
- * @param tag The name of the tag.
+ * @param tags The name of the tag.
Review Comment:
comment need updating
##########
clients/cli/src/main/java/org/apache/gravitino/cli/commands/UntagEntity.java:
##########
@@ -103,12 +103,13 @@ public void handle() {
return;
}
- String all = String.join(",", tags);
+ String all = String.join(",", removeTags);
if (all.equals("")) {
all = "nothing";
}
- System.out.println(entity + " removed tag " + tag + ", now tagged with " +
all);
+ System.out.println(
+ entity + " removed tag " + String.join(",", tags) + ", now tagged with
" + all);
Review Comment:
Perhaps change to "removed tag(s)"? Or put in tag or tags as needed?
##########
docs/cli.md:
##########
@@ -456,10 +457,10 @@ gcli group delete --group new_group
gcli tag details --tag tagA
```
-#### Create a tag
+#### Create tags
```bash
- gcli tag create --tag tagA
+ gcli tag create --tag tagA tagB
Review Comment:
This is beyond the scope of this PR, if you feel it is an issue, please
raise another issue for it. However, the CLI is using a typical pattern of
"entity verb" and then specifying other info with flags. See, for example,
https://clig.dev/#subcommands, and in general, flags are preferred to arguments
see https://clig.dev/#arguments-and-flags.
##########
docs/cli.md:
##########
@@ -468,22 +469,22 @@ gcli tag details --tag tagA
gcli tag list
```
-#### Delete a tag
+#### Delete tags
```bash
-gcli tag delete --tag tagA
+gcli tag delete --tag tagA tagB
```
-#### Add a tag to an entity
+#### Add tags to an entity
```bash
-gcli tag set --name catalog_postgres.hr --tag tagA
+gcli tag set --name catalog_postgres.hr --tag tagA tagB
Review Comment:
Note that by default the CLI by default (when multiple arguments are turned
on) accept "--tag tagA --tag tagB --tag tagC" or "--tag tagA tagB tagC", the
delimiter can also be changed from the default of space.
##########
docs/cli.md:
##########
@@ -456,10 +456,10 @@ gcli group delete --group new_group
gcli tag details --tag tagA
```
-#### Create a tag
+#### Create tags
```bash
- gcli tag create --tag tagA
+ gcli tag create --tag tagA tagB
Review Comment:
As explained before, this PR is to support multiple tags, not about the
command format. So, this is out of the scope of this PR, but feel free to raise
an issue about it.
##########
clients/cli/src/main/java/org/apache/gravitino/cli/GravitinoCommandLine.java:
##########
@@ -378,40 +381,44 @@ protected void handleTagCommand() {
}
} else if (CommandActions.CREATE.equals(command)) {
String comment = line.getOptionValue(GravitinoOptions.COMMENT);
- newCreateTag(url, ignore, metalake, tag, comment).handle();
+ newCreateTags(url, ignore, metalake, tags, comment).handle();
} else if (CommandActions.DELETE.equals(command)) {
boolean force = line.hasOption(GravitinoOptions.FORCE);
- newDeleteTag(url, ignore, force, metalake, tag).handle();
+ newDeleteTag(url, ignore, force, metalake, tags).handle();
} else if (CommandActions.SET.equals(command)) {
String property = line.getOptionValue(GravitinoOptions.PROPERTY);
String value = line.getOptionValue(GravitinoOptions.VALUE);
-
if (property != null && value != null) {
- newSetTagProperty(url, ignore, metalake, tag, property,
value).handle();
+ newSetTagProperty(url, ignore, metalake, getOneTag(tags), property,
value).handle();
} else if (name != null && property == null && value == null) {
- newTagEntity(url, ignore, metalake, name, tag).handle();
+ newTagEntity(url, ignore, metalake, name, tags).handle();
}
} else if (CommandActions.REMOVE.equals(command)) {
String property = line.getOptionValue(GravitinoOptions.PROPERTY);
if (property != null) {
- newRemoveTagProperty(url, ignore, metalake, tag, property).handle();
+ newRemoveTagProperty(url, ignore, metalake, getOneTag(tags),
property).handle();
} else {
- newUntagEntity(url, ignore, metalake, name, tag).handle();
+ newUntagEntity(url, ignore, metalake, name, tags).handle();
}
} else if (CommandActions.PROPERTIES.equals(command)) {
- newListTagProperties(url, ignore, metalake, tag).handle();
+ newListTagProperties(url, ignore, metalake, getOneTag(tags)).handle();
} else if (CommandActions.UPDATE.equals(command)) {
if (line.hasOption(GravitinoOptions.COMMENT)) {
String comment = line.getOptionValue(GravitinoOptions.COMMENT);
- newUpdateTagComment(url, ignore, metalake, tag, comment).handle();
+ newUpdateTagComment(url, ignore, metalake, getOneTag(tags),
comment).handle();
}
if (line.hasOption(GravitinoOptions.RENAME)) {
String newName = line.getOptionValue(GravitinoOptions.RENAME);
- newUpdateTagName(url, ignore, metalake, tag, newName).handle();
+ newUpdateTagName(url, ignore, metalake, getOneTag(tags),
newName).handle();
}
}
}
+ private String getOneTag(String[] tags) {
+ Preconditions.checkArgument(tags.length <= 1,
ErrorMessages.MULTIPLE_TAG_COMMAND_ERROR);
+ return tags[0];
Review Comment:
No tags can not be empty, the CLI library ensures that if you have a --tag
flag, it must have a value.
##########
clients/cli/src/main/java/org/apache/gravitino/cli/commands/CreateTag.java:
##########
@@ -35,34 +37,37 @@ public class CreateTag extends Command {
* @param url The URL of the Gravitino server.
* @param ignoreVersions If true don't check the client/server versions
match.
* @param metalake The name of the metalake.
- * @param tag The name of the tag.
+ * @param tags The name of the tag.
* @param comment The comment of the tag.
*/
public CreateTag(
- String url, boolean ignoreVersions, String metalake, String tag, String
comment) {
+ String url, boolean ignoreVersions, String metalake, String[] tags,
String comment) {
super(url, ignoreVersions);
this.metalake = metalake;
- this.tag = tag;
+ this.tags = tags;
this.comment = comment;
}
/** Create a new tag. */
@Override
public void handle() {
+ List<String> created = new ArrayList<>();
try {
GravitinoClient client = buildClient(metalake);
- client.createTag(tag, comment, null);
+ for (String tag : tags) {
+ client.createTag(tag, comment, null);
+ created.add(tag);
+ }
} catch (NoSuchMetalakeException err) {
System.err.println(ErrorMessages.UNKNOWN_METALAKE);
- return;
} catch (TagAlreadyExistsException err) {
System.err.println(ErrorMessages.TAG_EXISTS);
- return;
} catch (Exception exp) {
System.err.println(exp.getMessage());
- return;
+ } finally {
+ if (!created.isEmpty()) {
+ System.out.println(String.join(",", created) + " created");
+ }
Review Comment:
Please leave the code structure as is, there is no need for a finally block.
##########
clients/cli/src/main/java/org/apache/gravitino/cli/GravitinoCommandLine.java:
##########
@@ -378,40 +381,50 @@ protected void handleTagCommand() {
}
} else if (CommandActions.CREATE.equals(command)) {
String comment = line.getOptionValue(GravitinoOptions.COMMENT);
- newCreateTag(url, ignore, metalake, tag, comment).handle();
+ newCreateTag(url, ignore, metalake, getTags(tags), comment).handle();
} else if (CommandActions.DELETE.equals(command)) {
boolean force = line.hasOption(GravitinoOptions.FORCE);
- newDeleteTag(url, ignore, force, metalake, tag).handle();
+ newDeleteTag(url, ignore, force, metalake, getTags(tags)).handle();
} else if (CommandActions.SET.equals(command)) {
String property = line.getOptionValue(GravitinoOptions.PROPERTY);
String value = line.getOptionValue(GravitinoOptions.VALUE);
-
if (property != null && value != null) {
- newSetTagProperty(url, ignore, metalake, tag, property,
value).handle();
+ newSetTagProperty(url, ignore, metalake, getOneTag(tags), property,
value).handle();
} else if (name != null && property == null && value == null) {
- newTagEntity(url, ignore, metalake, name, tag).handle();
+ newTagEntity(url, ignore, metalake, name, getTags(tags)).handle();
}
} else if (CommandActions.REMOVE.equals(command)) {
String property = line.getOptionValue(GravitinoOptions.PROPERTY);
if (property != null) {
- newRemoveTagProperty(url, ignore, metalake, tag, property).handle();
+ newRemoveTagProperty(url, ignore, metalake, getOneTag(tags),
property).handle();
} else {
- newUntagEntity(url, ignore, metalake, name, tag).handle();
+ newUntagEntity(url, ignore, metalake, name, getTags(tags)).handle();
}
} else if (CommandActions.PROPERTIES.equals(command)) {
- newListTagProperties(url, ignore, metalake, tag).handle();
+ newListTagProperties(url, ignore, metalake, getOneTag(tags)).handle();
} else if (CommandActions.UPDATE.equals(command)) {
if (line.hasOption(GravitinoOptions.COMMENT)) {
String comment = line.getOptionValue(GravitinoOptions.COMMENT);
- newUpdateTagComment(url, ignore, metalake, tag, comment).handle();
+ newUpdateTagComment(url, ignore, metalake, getOneTag(tags),
comment).handle();
}
if (line.hasOption(GravitinoOptions.RENAME)) {
String newName = line.getOptionValue(GravitinoOptions.RENAME);
- newUpdateTagName(url, ignore, metalake, tag, newName).handle();
+ newUpdateTagName(url, ignore, metalake, getOneTag(tags),
newName).handle();
}
}
}
+ private String[] getTags(String[] tags) {
+ Preconditions.checkArgument(tags != null, ErrorMessages.TAG_EMPTY);
+ return tags;
+ }
+
+ private String getOneTag(String[] tags) {
+ Preconditions.checkArgument(tags != null, ErrorMessages.TAG_EMPTY);
+ Preconditions.checkArgument(tags.length <= 1,
ErrorMessages.MULTIPLE_TAG_COMMAND_ERROR);
+ return tags != null ? tags[0] : null;
Review Comment:
You already have the precondition check above is this null check needed?
##########
clients/cli/src/main/java/org/apache/gravitino/cli/commands/DeleteTag.java:
##########
@@ -38,42 +41,45 @@ public class DeleteTag extends Command {
* @param ignoreVersions If true don't check the client/server versions
match.
* @param force Force operation.
* @param metalake The name of the metalake.
- * @param tag The name of the tag.
+ * @param tags The name of the tag.
*/
- public DeleteTag(String url, boolean ignoreVersions, boolean force, String
metalake, String tag) {
+ public DeleteTag(
+ String url, boolean ignoreVersions, boolean force, String metalake,
String[] tags) {
super(url, ignoreVersions);
this.force = force;
this.metalake = metalake;
- this.tag = tag;
+ this.tags = tags;
}
/** Delete a tag. */
@Override
public void handle() {
- boolean deleted = false;
-
if (!AreYouSure.really(force)) {
return;
}
-
+ List<String> deleted = new ArrayList<>();
try {
GravitinoClient client = buildClient(metalake);
- deleted = client.deleteTag(tag);
+ for (String tag : tags) {
+ if (client.deleteTag(tag)) {
+ deleted.add(tag);
+ }
+ }
} catch (NoSuchMetalakeException err) {
System.err.println(ErrorMessages.UNKNOWN_METALAKE);
- return;
} catch (NoSuchTagException err) {
System.err.println(ErrorMessages.UNKNOWN_TAG);
- return;
} catch (Exception exp) {
System.err.println(exp.getMessage());
- return;
- }
-
- if (deleted) {
- System.out.println(tag + " deleted.");
- } else {
- System.out.println(tag + " not deleted.");
+ } finally {
+ if (!deleted.isEmpty()) {
+ System.out.println(String.join(",", deleted) + " deleted.");
+ if (deleted.size() < tags.length) {
+ List<String> remaining = Arrays.asList(tags);
+ remaining.removeAll(deleted);
+ System.out.println(String.join(",", remaining) + " not deleted.");
+ }
+ }
Review Comment:
As above, please keep structure as is.
##########
clients/cli/src/main/java/org/apache/gravitino/cli/GravitinoCommandLine.java:
##########
@@ -366,10 +368,11 @@ protected void handleTagCommand() {
String url = getUrl();
FullName name = new FullName(line);
String metalake = name.getMetalakeName();
- String tag = line.getOptionValue(GravitinoOptions.TAG);
+ String[] tags = line.getOptionValues(GravitinoOptions.TAG);
Review Comment:
Have you tested this? I think you may get a CLI error if TAG has no value,
so this extra code below may not be needed.
##########
clients/cli/src/test/java/org/apache/gravitino/cli/TestMain.java:
##########
@@ -131,6 +133,74 @@ public void parseError() throws
UnsupportedEncodingException {
assertTrue(outContent.toString().contains("usage:")); // Expect help output
}
+ @Test
+ public void tagWithMultiArgs() throws ParseException {
+ Options options = new GravitinoOptions().options();
+ CommandLineParser parser = new DefaultParser();
+
+ // gcli tag create --tag tagA tagB
+ String[] args = {"tag", "create", "--tag", "tagA", "tagB"};
+ CommandLine line = parser.parse(options, args);
+ String[] tags = line.getOptionValues("tag");
+ assertArrayEquals(tags, new Object[] {"tagA", "tagB"});
+
+ // gcli tag delete --tag tagA tagB
+ String[] deleteArgs = {"tag", "delete", "--tag", "tagA", "tagB"};
+ CommandLine deleteLine = parser.parse(options, deleteArgs);
+ String[] deleteTags = deleteLine.getOptionValues("tag");
+ assertArrayEquals(deleteTags, new Object[] {"tagA", "tagB"});
+
+ // gcli tag set --name catalog_postgres.hr --tag tagA tagB
+ String[] setArgs = {"tag", "set", "--name", "catalog_postgres.hr",
"--tag", "tagA", "tagB"};
+ CommandLine setLine = parser.parse(options, setArgs);
+ String[] setTags = setLine.getOptionValues("tag");
+ assertArrayEquals(setTags, new Object[] {"tagA", "tagB"});
+
+ // gcli tag remove --name catalog_postgres.hr --tag tagA tagB
+ String[] removeArgs = {
+ "tag", "remove", "--name", "catalog_postgres.hr", "--tag", "tagA", "tagB"
+ };
+ CommandLine removeLine = parser.parse(options, removeArgs);
+ String[] removeTags = removeLine.getOptionValues("tag");
+ assertArrayEquals(removeTags, new Object[] {"tagA", "tagB"});
+ }
Review Comment:
This test doesn't really test anything outside of the internals of the CLI
library so I'm not sure of it's value.
##########
docs/cli.md:
##########
@@ -44,6 +44,7 @@ The general structure for running commands with the Gravitino
CLI is `gcli entit
--rename <arg> new entity name
-s,--server Gravitino server version
-t,--tag <arg> tag name
+ -t,--tags <arg> List tag names
Review Comment:
We should have one flag for tags
##########
clients/cli/src/main/java/org/apache/gravitino/cli/commands/DeleteTag.java:
##########
@@ -28,52 +31,55 @@
public class DeleteTag extends Command {
protected final String metalake;
- protected final String tag;
+ protected final String[] tags;
protected final boolean force;
/**
- * Delete a tag.
+ * Delete tags.
*
* @param url The URL of the Gravitino server.
* @param ignoreVersions If true don't check the client/server versions
match.
* @param force Force operation.
* @param metalake The name of the metalake.
- * @param tag The name of the tag.
+ * @param tags The names of the tags.
*/
- public DeleteTag(String url, boolean ignoreVersions, boolean force, String
metalake, String tag) {
+ public DeleteTag(
+ String url, boolean ignoreVersions, boolean force, String metalake,
String[] tags) {
super(url, ignoreVersions);
this.force = force;
this.metalake = metalake;
- this.tag = tag;
+ this.tags = tags;
}
- /** Delete a tag. */
+ /** Delete tags. */
@Override
public void handle() {
- boolean deleted = false;
-
if (!AreYouSure.really(force)) {
return;
}
-
+ List<String> deleted = new ArrayList<>();
try {
GravitinoClient client = buildClient(metalake);
- deleted = client.deleteTag(tag);
+ for (String tag : tags) {
+ if (client.deleteTag(tag)) {
+ deleted.add(tag);
+ }
+ }
} catch (NoSuchMetalakeException err) {
System.err.println(ErrorMessages.UNKNOWN_METALAKE);
- return;
} catch (NoSuchTagException err) {
System.err.println(ErrorMessages.UNKNOWN_TAG);
- return;
} catch (Exception exp) {
System.err.println(exp.getMessage());
- return;
- }
-
- if (deleted) {
- System.out.println(tag + " deleted.");
- } else {
- System.out.println(tag + " not deleted.");
+ } finally {
+ if (!deleted.isEmpty()) {
+ System.out.println(String.join(",", deleted) + " deleted.");
Review Comment:
If you delete just one tag, the suggested change is not correct. Also, this
should be consistent with what other delete commands output.
##########
clients/cli/src/main/java/org/apache/gravitino/cli/commands/DeleteTag.java:
##########
@@ -28,52 +31,55 @@
public class DeleteTag extends Command {
protected final String metalake;
- protected final String tag;
+ protected final String[] tags;
protected final boolean force;
/**
- * Delete a tag.
+ * Delete tags.
*
* @param url The URL of the Gravitino server.
* @param ignoreVersions If true don't check the client/server versions
match.
* @param force Force operation.
* @param metalake The name of the metalake.
- * @param tag The name of the tag.
+ * @param tags The names of the tags.
*/
- public DeleteTag(String url, boolean ignoreVersions, boolean force, String
metalake, String tag) {
+ public DeleteTag(
+ String url, boolean ignoreVersions, boolean force, String metalake,
String[] tags) {
super(url, ignoreVersions);
this.force = force;
this.metalake = metalake;
- this.tag = tag;
+ this.tags = tags;
}
- /** Delete a tag. */
+ /** Delete tags. */
@Override
public void handle() {
- boolean deleted = false;
-
if (!AreYouSure.really(force)) {
return;
}
-
+ List<String> deleted = new ArrayList<>();
try {
GravitinoClient client = buildClient(metalake);
- deleted = client.deleteTag(tag);
+ for (String tag : tags) {
+ if (client.deleteTag(tag)) {
+ deleted.add(tag);
+ }
+ }
} catch (NoSuchMetalakeException err) {
System.err.println(ErrorMessages.UNKNOWN_METALAKE);
- return;
} catch (NoSuchTagException err) {
System.err.println(ErrorMessages.UNKNOWN_TAG);
- return;
} catch (Exception exp) {
System.err.println(exp.getMessage());
- return;
- }
-
- if (deleted) {
- System.out.println(tag + " deleted.");
- } else {
- System.out.println(tag + " not deleted.");
+ } finally {
+ if (!deleted.isEmpty()) {
+ System.out.println(String.join(",", deleted) + " deleted.");
+ if (deleted.size() < tags.length) {
+ List<String> remaining = Arrays.asList(tags);
+ remaining.removeAll(deleted);
+ System.out.println(String.join(",", remaining) + " not deleted.");
Review Comment:
Same as above "Tags" doesn't work if only one tag is remaining.
##########
clients/cli/src/main/java/org/apache/gravitino/cli/GravitinoCommandLine.java:
##########
@@ -378,40 +381,50 @@ protected void handleTagCommand() {
}
} else if (CommandActions.CREATE.equals(command)) {
String comment = line.getOptionValue(GravitinoOptions.COMMENT);
- newCreateTag(url, ignore, metalake, tag, comment).handle();
+ newCreateTag(url, ignore, metalake, getTags(tags), comment).handle();
} else if (CommandActions.DELETE.equals(command)) {
boolean force = line.hasOption(GravitinoOptions.FORCE);
- newDeleteTag(url, ignore, force, metalake, tag).handle();
+ newDeleteTag(url, ignore, force, metalake, getTags(tags)).handle();
} else if (CommandActions.SET.equals(command)) {
String property = line.getOptionValue(GravitinoOptions.PROPERTY);
String value = line.getOptionValue(GravitinoOptions.VALUE);
-
if (property != null && value != null) {
- newSetTagProperty(url, ignore, metalake, tag, property,
value).handle();
+ newSetTagProperty(url, ignore, metalake, getOneTag(tags), property,
value).handle();
} else if (name != null && property == null && value == null) {
- newTagEntity(url, ignore, metalake, name, tag).handle();
+ newTagEntity(url, ignore, metalake, name, getTags(tags)).handle();
}
} else if (CommandActions.REMOVE.equals(command)) {
String property = line.getOptionValue(GravitinoOptions.PROPERTY);
if (property != null) {
- newRemoveTagProperty(url, ignore, metalake, tag, property).handle();
+ newRemoveTagProperty(url, ignore, metalake, getOneTag(tags),
property).handle();
} else {
- newUntagEntity(url, ignore, metalake, name, tag).handle();
+ newUntagEntity(url, ignore, metalake, name, getTags(tags)).handle();
}
} else if (CommandActions.PROPERTIES.equals(command)) {
- newListTagProperties(url, ignore, metalake, tag).handle();
+ newListTagProperties(url, ignore, metalake, getOneTag(tags)).handle();
} else if (CommandActions.UPDATE.equals(command)) {
if (line.hasOption(GravitinoOptions.COMMENT)) {
String comment = line.getOptionValue(GravitinoOptions.COMMENT);
- newUpdateTagComment(url, ignore, metalake, tag, comment).handle();
+ newUpdateTagComment(url, ignore, metalake, getOneTag(tags),
comment).handle();
}
if (line.hasOption(GravitinoOptions.RENAME)) {
String newName = line.getOptionValue(GravitinoOptions.RENAME);
- newUpdateTagName(url, ignore, metalake, tag, newName).handle();
+ newUpdateTagName(url, ignore, metalake, getOneTag(tags),
newName).handle();
}
}
}
+ private String[] getTags(String[] tags) {
+ Preconditions.checkArgument(tags != null, ErrorMessages.TAG_EMPTY);
Review Comment:
I'm not sure it is possible to have tags as null as the CLI make it a
required option. It would be best to test this.
##########
docs/cli.md:
##########
@@ -468,22 +469,22 @@ gcli tag details --tag tagA
gcli tag list
```
-#### Delete a tag
+#### Delete tags
```bash
-gcli tag delete --tag tagA
+gcli tag delete --tag tagA tagB
```
-#### Add a tag to an entity
+#### Add tags to an entity
```bash
-gcli tag set --name catalog_postgres.hr --tag tagA
+gcli tag set --name catalog_postgres.hr --tag tagA tagB
Review Comment:
Out of scope for this PR.
##########
clients/cli/src/main/java/org/apache/gravitino/cli/commands/CreateTag.java:
##########
@@ -19,39 +19,51 @@
package org.apache.gravitino.cli.commands;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.List;
import org.apache.gravitino.cli.ErrorMessages;
import org.apache.gravitino.client.GravitinoClient;
import org.apache.gravitino.exceptions.NoSuchMetalakeException;
import org.apache.gravitino.exceptions.TagAlreadyExistsException;
public class CreateTag extends Command {
protected final String metalake;
- protected final String tag;
+ protected final String[] tags;
protected final String comment;
/**
- * Create a new tag.
+ * Create tags.
*
* @param url The URL of the Gravitino server.
* @param ignoreVersions If true don't check the client/server versions
match.
* @param metalake The name of the metalake.
- * @param tag The name of the tag.
+ * @param tags The names of the tags.
* @param comment The comment of the tag.
*/
public CreateTag(
- String url, boolean ignoreVersions, String metalake, String tag, String
comment) {
+ String url, boolean ignoreVersions, String metalake, String[] tags,
String comment) {
super(url, ignoreVersions);
this.metalake = metalake;
- this.tag = tag;
+ this.tags = tags;
Review Comment:
That's really up to the Java API the CLI calls, the CLI is a thin wrapper on
that.
##########
clients/cli/src/main/java/org/apache/gravitino/cli/GravitinoCommandLine.java:
##########
@@ -366,10 +368,11 @@ protected void handleTagCommand() {
String url = getUrl();
FullName name = new FullName(line);
String metalake = name.getMetalakeName();
- String tag = line.getOptionValue(GravitinoOptions.TAG);
+ String[] tags = line.getOptionValues(GravitinoOptions.TAG);
+ tags = tags != null ?
Arrays.stream(tags).distinct().toArray(String[]::new) : null;
Review Comment:
Fixed
##########
clients/cli/src/test/java/org/apache/gravitino/cli/TestMain.java:
##########
@@ -131,6 +133,74 @@ public void parseError() throws
UnsupportedEncodingException {
assertTrue(outContent.toString().contains("usage:")); // Expect help output
}
+ @Test
+ public void tagWithMultiArgs() throws ParseException {
+ Options options = new GravitinoOptions().options();
+ CommandLineParser parser = new DefaultParser();
+
+ // gcli tag create --tag tagA tagB
+ String[] args = {"tag", "create", "--tag", "tagA", "tagB"};
+ CommandLine line = parser.parse(options, args);
+ String[] tags = line.getOptionValues("tag");
+ assertArrayEquals(tags, new Object[] {"tagA", "tagB"});
+
+ // gcli tag delete --tag tagA tagB
+ String[] deleteArgs = {"tag", "delete", "--tag", "tagA", "tagB"};
+ CommandLine deleteLine = parser.parse(options, deleteArgs);
+ String[] deleteTags = deleteLine.getOptionValues("tag");
+ assertArrayEquals(deleteTags, new Object[] {"tagA", "tagB"});
+
+ // gcli tag set --name catalog_postgres.hr --tag tagA tagB
+ String[] setArgs = {"tag", "set", "--name", "catalog_postgres.hr",
"--tag", "tagA", "tagB"};
+ CommandLine setLine = parser.parse(options, setArgs);
+ String[] setTags = setLine.getOptionValues("tag");
+ assertArrayEquals(setTags, new Object[] {"tagA", "tagB"});
+
+ // gcli tag remove --name catalog_postgres.hr --tag tagA tagB
+ String[] removeArgs = {
+ "tag", "remove", "--name", "catalog_postgres.hr", "--tag", "tagA", "tagB"
+ };
+ CommandLine removeLine = parser.parse(options, removeArgs);
+ String[] removeTags = removeLine.getOptionValues("tag");
+ assertArrayEquals(removeTags, new Object[] {"tagA", "tagB"});
+ }
Review Comment:
It would be better to add similar tests to TestTagCommands.java.
--
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]
