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

[#5831] fix(CLI): Fix CLi gives unexpected output when input tag set command #5897

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Abyss-lord
Copy link
Contributor

@Abyss-lord Abyss-lord commented Dec 18, 2024

What changes were proposed in this pull request?

Running the command gcli tag set --metalake metalake_demo or gcli tag remove --metalake metalake_demo gives unexpected output.it should give some help information.

Why are the changes needed?

Fix: #5831

Does this PR introduce any user-facing change?

NO

How was this patch tested?

bin/gcli.sh tag set --metalake demo_metalake 
# output: Malformed entity name.

bin/gcli.sh tag set --tag role1 --property test
# output: This command cannot be executed. The tag set command only supports configuring tag properties or attaching tags to entities.

bin/gcli.sh tag set --tag role1 --value val1
# output: This command cannot be executed. The tag set command only supports configuring tag properties or attaching tags to entities.

bin/gcli.sh tag remove --metalake demo_metalake 
# output: Malformed entity name.

bin/gcli.sh tag set --metalake demo_metalake --name Hive_catalog --tag tagA tagB
# output: Hive_catalog now tagged with tagB,tagA

bin/gcli.sh tag remove --metalake demo_metalake --name Hive_catalog
# output: Hive_catalog removed tags tagB,tagA now tagged with nothing

@Abyss-lord
Copy link
Contributor Author

hi @justinmclean @xunliu , Could you please take a look when you have time?

@@ -811,6 +826,16 @@ private void handleFilesetCommand() {
}
}

private boolean hasEntity(FullName name) {
Copy link
Member

@justinmclean justinmclean Dec 18, 2024

Choose a reason for hiding this comment

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

The logic here is not correct as for instance a table name can be null if you are dealing with a schema or a catalog.

@Abyss-lord Abyss-lord force-pushed the fix-tag-command branch 2 times, most recently from 2857789 to 328bc1f Compare December 19, 2024 05:53
…a tag

Running the command gcli tag set --metalake metalake_demo or gcli tag remove --metalake metalake_demo gives unexpected output.it should give some help information.
…a tag

fix some problems according to reviewer.
@justinmclean
Copy link
Member

justinmclean commented Dec 19, 2024

This is not making 100% sense to me. There are a couple of things I think can be improved here:

  • The output of bin/gcli.sh tag set --metalake demo_metalake should be Missing --name option. as the name argument is missing. The existing code should already do this before this function is called?
  • The hasEntity logic seems wrong, and I am not sure what you are trying to check here. Could teh function have a better name?
  • In hasEntity I'm not sure why you are checking 3 parts of the name when all sorts of entities can be tagged. Also columns will have a four part name.
  • Also there are hasCatalogName, hasTableName, etc methods that would be better to use, if where needed to be used?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Improvement] Setting a tag property in the Gravitino CLi gives unexpected output
2 participants