Allow to manage multiple connectors when it apply #167

jvenant · 2022-03-04T22:56:58Z

Fixes #107

This is a base of discussion about the best way to manage multiples connectors in one command
For example, I choose to add a --all option to process all connectors. But it could be a good idea to allow regexp or wildcard pattern instead (or both)
For apply, I choose to not allow -n with multiple -f because it can be tricky to use otherwise. It might be too restrictive.
Just let me know if it's going the good way. I can adapt the implementation following recommendations.
And then apply the changes on all the other commands where it applies.

gunnarmorling

Hey @jvenant, this looks great! Some minor comments/suggestions inline.

For example, I choose to add a --all option to process all connectors. But it could be a good idea to allow regexp or wildcard pattern instead (or both)

Good question; in general, I prefer if there's one way to acomplish a given task, not multiple ones. So if you foresee the need for selecting names using regexps, I wouldn't add --all, because that'd be the same as --name-pattern=.*. Given regexps are more flexible and "all" is just a special case, I'd probably go for the more powerful variant and add --name-pattern.

For apply, I choose to not allow -n with multiple -f because it can be tricky to use otherwise. It might be too restrictive.

I think that's very sensible. Let's do it that way.

Just let me know if it's going the good way. I can adapt the implementation following recommendations.
And then apply the changes on all the other commands where it applies.

+1 I think you're on the right track. Thanks a lot!

Once we've converged on the options, the completion script should be updated as well (see README for instructions).

src/main/java/org/kcctl/command/ApplyCommand.java

gunnarmorling · 2022-03-05T10:30:15Z

src/main/java/org/kcctl/command/ApplyCommand.java

        String contents;
        if (file.getName().equals("-")) {
            contents = readFromStdin();
        }
        else if (!file.exists()) {
-            spec.commandLine().getOut().println("Given file does not exist: " + file.toPath().toAbsolutePath());
-            return 1;
+            throw new CommandLine.ExecutionException(spec.commandLine(), "Given file does not exist: " + file.toPath().toAbsolutePath());


What's the reason for raising an exception rather than returning a none 0 value?

Good question, I asked to myself if it was a good idea.
The pragmatic answer is because the method return a String ^_^
But it can be manage of course
The underneath question is more global. It is about how to manage error when multiple connectors are processed.

Should we run everything and then report all errors at the end ?

Should we stop on first error

But for the readConfig specific case, maybe it would be smarter to first read all the files and then only launch the apply process if evrything is ok.

What do you think ?

But for the readConfig specific case, maybe it would be smarter to first read all the files and then only launch the apply process if evrything is ok.

Yes, I think that makes sense. In general, I would

Try to do as much validation/verification and abort if there was any validation failure, before starting mutating actions

Abort after the first failure of a mutating action

src/main/java/org/kcctl/command/PauseConnectorCommand.java

jvenant · 2022-03-05T13:12:12Z

for the regexp stuff : I was thinking about a -e option that would then treat the names as regexp filters on all connectors names (like grep) instead of --name-pattern=.*.

jvenant · 2022-03-05T15:32:54Z

I'm facing difficulties generating the completion. Picocli seems to (currently) requires an empty constructor. Which is not compatible with the constructror injection pattern used on some commands

gunnarmorling · 2022-03-05T16:23:20Z

Picocli seems to (currently) requires an empty constructor. Which is not compatible with the constructror injection pattern used on some commands

Yes, that's true. I thought I'd have added those default constructors to all the commands, but I may have missed some. Just add them in this case. They'll only be used for the completion generation, otherwise the inject-ed constructors are used.

gunnarmorling · 2022-03-05T16:25:05Z

for the regexp stuff : I was thinking about a -e option that would then treat the names as regexp filters on all connectors names (like grep) instead of --name-pattern=.*.

So would -e take the pattern then (which I think grep is doing)? Or would -e be a "switch" that impacts how the --name value is interpreted? Can you share the entire command invocation perhaps?

jvenant · 2022-03-05T17:22:05Z

for the regexp stuff : I was thinking about a -e option that would then treat the names as regexp filters on all connectors names (like grep) instead of --name-pattern=.*.

So would -e take the pattern then (which I think grep is doing)? Or would -e be a "switch" that impacts how the --name value is interpreted? Can you share the entire command invocation perhaps?

Good point : Not exactly like grep then. I see it more like a switch ;
kcctl pause connector -e 'topic\.\d' 'othertopic\.\d'
vs
kcctl pause connector topic.1 topic.2 othertopic.1 othertopic.2 ...

It could be confusing as the user may consider the -e as a tag for the name just after. But I feel like it's more compliant with the way the commands are currently designed.
tell me your preference :

--name-pattern before 'normal' names : then we have to decide how to manage mix of --name-pattern and 'normal' names argument
-e like grep : might be tricky to manage commands like -e 'reg1' 'name1' -e 'reg2' 'name2'
-e like a switch

jvenant · 2022-03-05T17:22:31Z

Picocli seems to (currently) requires an empty constructor. Which is not compatible with the constructror injection pattern used on some commands

Yes, that's true. I thought I'd have added those default constructors to all the commands, but I may have missed some. Just add them in this case. They'll only be used for the completion generation, otherwise the inject-ed constructors are used.

I'll do
Separated PR ?

jvenant · 2022-03-05T18:21:29Z

I made a commit taking in account your feedback. Currently it is a switch -e on pause, but I can change to something else.

I redesign the patch for multiple files on Apply. I think it is more robust like this, but it involve more changes in the code that I was expecting. So don't hesitate to tell me if you're not OK with some of my changes. I can adapt.

I'll apply the changes on others commands once we are OK with this ones

gunnarmorling · 2022-03-06T09:34:31Z

Currently it is a switch -e on pause, but I can change to something else.

Cool, I think that's a good approach. I was leaning towards --name-pattern initially, since I (incorrectly) assumed that the connector name would currently be given via --name, and I found it confusing if one option would impact another option. But instead, the name is just given after the command (i.e. a parameter and not an option in PicoCli terms). So having an option like --regexp which controls how the parameter is interpreted makes sense 👍 .

My only question would be why "-e"? I.e. what does "e" it stand for, "expression"? How about using "-r"/"--reg-exp" for easier memorization? Btw. on "--reg-exp" vs. "--regexp", it probably should be the former, as we use the dash in multi-word option names (e.g. see SetContextCommand).

gunnarmorling · 2022-03-06T09:38:40Z

I redesign the patch for multiple files on Apply. I think it is more robust like this, but it involve more changes in the code that I was expecting. So don't hesitate to tell me if you're not OK with some of my changes. I can adapt.

Thanks, it all LGTM and I think we've achieved agreement on the way forward. Apart from bringing regexp support to all commands taking a connector or task name, one other thing to do would be updating the list of commands in the README (it's the out of kcctl without any commands).

Ah, and at the end, ideally everything should be squashed into one commit, using a suitable gitmoji emoji and the issue key as prefix (see other commits in the history for examples). Thanks again!

jvenant · 2022-03-06T11:15:45Z

Patch already have a -r option

jvenant · 2022-03-06T12:51:21Z

I currently setup -r and --reg-exp on all commands except Patch who is -e --reg-exp
probably not the best solution
Let me know how you want to deal with the -r short option conflict

gunnarmorling · 2022-03-06T19:39:43Z

Patch already have a -r option

Ah yes, I see. So looks like we should use -e / --reg-exp consistently for all commands then. Using different short names for the same option for different commands is too inconsistent and confusing.

jvenant · 2022-03-07T20:13:40Z

Changed to -e everywhere.
Still I have one issue left. I can't build native binary. Graalvm complains about :
Error: com.oracle.svm.hosted.substitute.DeletedElementException: Unsupported method java.lang.Class.getConstantPool() is reachable: The declaring class of this element has been substituted, but this element is not present in the substitution class

Any idea ?

gunnarmorling · 2022-03-07T21:03:03Z

Interesting, I haven't seen that one before. Are you using Java 17 to build kcctl=

…

Message ID: ***@***.***>

jvenant · 2022-03-08T06:35:18Z

Yes java 17. It works without my commit.
I'll dig then

jvenant · 2022-03-08T19:29:12Z

It's related to the ApplyConnector record I added in ApplyCommand.
Graalvm doesn't seem to like when it is part of a class field type
So I changed the applyConnectors type from ApplyConnector[] to List (changing the visibility of the record doesn't help. List<ApplyConnector> neither)
It's a little bit uggly as it force to cast. But it should do the job

So : I think we're good...
Tell me if you see anything

gunnarmorling · 2022-03-13T09:40:00Z

Hey @jvenant, so this LGTM, merging. Thanks a lot for this very nice piece of work!

gunnarmorling reviewed Mar 5, 2022

View reviewed changes

jvenant added a commit to jvenant/kcctl that referenced this pull request Mar 6, 2022

✨ kcctl#167 allow multiple connector names and regexp

357873f

jvenant force-pushed the main branch from f8ecd56 to 357873f Compare March 6, 2022 12:46

jvenant marked this pull request as ready for review March 6, 2022 12:47

jvenant added a commit to jvenant/kcctl that referenced this pull request Mar 7, 2022

✨ kcctl#167 allow multiple connector names and regexp

13aa7f1

jvenant force-pushed the main branch from 357873f to 13aa7f1 Compare March 7, 2022 20:02

jvenant force-pushed the main branch from 13aa7f1 to 44861aa Compare March 8, 2022 19:22

✨ kcctl#167 allow multiple connector names and regexp

44861aa

gunnarmorling merged commit 2e8980a into kcctl:main Mar 13, 2022

C0urante mentioned this pull request Oct 13, 2022

kcctl apply -f should allow multiple files #250

Closed

Allow to manage multiple connectors when it apply #167

Allow to manage multiple connectors when it apply #167

Uh oh!

Conversation

jvenant commented Mar 4, 2022

Uh oh!

gunnarmorling left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

gunnarmorling Mar 5, 2022

Choose a reason for hiding this comment

Uh oh!

jvenant Mar 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gunnarmorling Mar 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jvenant commented Mar 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jvenant commented Mar 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gunnarmorling commented Mar 5, 2022

Uh oh!

gunnarmorling commented Mar 5, 2022

Uh oh!

jvenant commented Mar 5, 2022

Uh oh!

jvenant commented Mar 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jvenant commented Mar 5, 2022

Uh oh!

gunnarmorling commented Mar 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gunnarmorling commented Mar 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jvenant commented Mar 6, 2022

Uh oh!

jvenant commented Mar 6, 2022

Uh oh!

gunnarmorling commented Mar 6, 2022

Uh oh!

jvenant commented Mar 7, 2022

Uh oh!

gunnarmorling commented Mar 7, 2022 via email

Uh oh!

jvenant commented Mar 8, 2022

Uh oh!

jvenant commented Mar 8, 2022

Uh oh!

gunnarmorling commented Mar 13, 2022

Uh oh!

Uh oh!

gunnarmorling left a comment •

edited

Loading

jvenant Mar 5, 2022 •

edited

Loading

gunnarmorling Mar 5, 2022 •

edited

Loading

jvenant commented Mar 5, 2022 •

edited

Loading

jvenant commented Mar 5, 2022 •

edited

Loading

jvenant commented Mar 5, 2022 •

edited

Loading

gunnarmorling commented Mar 6, 2022 •

edited

Loading

gunnarmorling commented Mar 6, 2022 •

edited

Loading