shaofengshi commented on code in PR #5293:
URL: https://github.com/apache/gravitino/pull/5293#discussion_r1848004206
##########
docs/cli.md:
##########
@@ -506,6 +507,26 @@ gcli tag update --tag tagA --rename newTag
gcli tag update --tag tagA --comment "new comment"
```
+### Owners commands
+
+#### List an owner
+
+```bash
+gcli catalog details --name postgres --owner
+```
+
+#### Set an owner to a user
+
+```bash
+gcli catalog set --name postgres --user admin
Review Comment:
> `set-owner` doesn't align with that advice as it's inconsistent with the
verbs used for different types of objects. i.e. you are using different verbs
for different objects. Also, in this case, more sub-commands mean that the code
needs to be duplicated for every entity rather than working off a single flag.
Having a single flag means it will be future-proof, as if new entities are
added as no new code is needed, while using a command will mean new code will
need to be added for each new entity added.
Use sub-command can also avoid duplicating code in every entity, that is not
the key point, it is similar with how you dealing with the flag now. E.g,
putting the `if("set-owner".equals(command)) {...}` in the same location of
current checking for "--owner" flag. It is the same.
Anyway, Justin I see your point, you perfer to using flags to checking
user's purpose, instead of using commands. I prefer to using more commands,
because in each command line, there is only one command be allowed, but
multiple flags can be set. So if using more flags, the logic of checking flags
will be complex and easy to cause problem; If multiple flags appeared in the
same time, you have to carefully decide which one can be executed and which one
won't, and you'd better give some warning message to the user.
Here I can give an example, in the current version, with the "gcli tag set"
command, user can use it to 1) set a property to the tag (with "--tag",
"--property" and "--value"), and 2) set the tag to an entity (with "--name" and
"--tag"). The logic is here:
https://github.com/apache/gravitino/blob/main/clients/cli/src/main/java/org/apache/gravitino/cli/GravitinoCommandLine.java#L452
Actually, this piece of code has several bugs. The IDE is clever and
highlighted them, but before today, you and me didn't aware that:
<img width="1037" alt="image"
src="https://github.com/user-attachments/assets/95a2dc4c-4415-4a65-8e9c-cb0989c2a390";>
That's why I suggested to use "set-tag" command instead of combining it with
the "set" command when reviewing the previous PR. Now comes the setting "owner"
function, that makes me re-think this design. Besides, I don't think "set-tag"
or "set-owner" command would make user hard to remember, as they are quite
simple words. ('Miller's Law' is not MUST, right?)
Anyway, if you insist that, I'm also okay.
--
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: commits-unsubscr...@gravitino.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
