Attributes defined in jbang-catalog.json should but appended to the end of the command.

[bartoszpop]

Describe the bug
Attributes defined in jbang-catalog.json should but appended to the end of the command.

To Reproduce
The below jbang-catalog.json results with

1. Create the following jbang-catalog.json
   {
   "aliases": {
   "camel": {
   "script-ref": "org.apache.camel:camel-jbang-main:4.8.3",
   "main": "main.CamelJBang",
   "arguments": ["--quarkus-version=3.16.0"]
   }
   }
   }

2. run the command jbang camel@apache/camel run * --runtime=quarkus. It will produce jbang9236616212127187886.args

-classpath
"(...)"
main.CamelJBang
--quarkus-version=3.17.0 <--- this should go at the end
run
.camel-jbang
.camel-jbang-run
somefile.camel.yaml
jbang-catalog.json
--runtime=quarkus

JBang version
[jbang] [0:664] jbang version 0.121.0

[quintesse]

Well... I understand that for your use-case you want it like that, but I think this is actually worse (besides the possibility that we could break existing code). The current implementation is based on how aliases work in shells (bash, zsg, etc), where user supplied arguments are also appended to the end. Why? So the user can override a parameter supplied by the alias, if alias params are put at the end they would always have precedence and the user would be much more limited.

A possible work-around for this would be to allow the Camel CLI to get its quarkus version from a system property, eg:

{
   "aliases": {
      "camel": {
         "script-ref": "org.apache.camel:camel-jbang-main:4.8.3",
         "main": "main.CamelJBang",
         "arguments": [],
         "properties": {
            "quarkus.version": "3.17.0"
         }
      }
   }
}

So you'd get something like:

-classpath
"(...)"
-Dquarkus.version=3.17.0
main.CamelJBang
run
.camel-jbang
.camel-jbang-run
somefile.camel.yaml
jbang-catalog.json
--runtime=quarkus

(You would still allow "--quarkus-version" as well of course, so users can change it if they want to)

[quintesse]

Oh, I just thought of another work-around.... except that it doesn't work, which does seem a bug. This is what I thought you could do:

Create 2 aliases, with one referring to the other, eg:

{
   "aliases": {
      "camel": {
         "script-ref": "camel-run",
         "arguments": ["--quarkus-version=3.16.0"]
      },
      "camel-run": {
         "script-ref": "org.apache.camel:camel-jbang-main:4.8.3",
         "main": "main.CamelJBang",
         "arguments": ["run"]
      }
   }
}

But it turns out the arguments of one alias overwrite the arguments from the other, which is unfortunate and I don't think that's correct. So I'll try to fix that.

[quintesse]

Created PR #1913

[bartoszpop]

@quintesse thank you for the fix.

[bartoszpop]

Hi @quintesse, Camel team is reluctant to make it configurable with a property that actually makes sense as it seems to be a limitation in JBang itself. Could we introduce a new parameter to append custom arguments at the end? Otherwise it will not be possible to pass custom arguments if the target script uses Picocli as the command should come next. WDYT? PFA a pr with the proposed change.

[quintesse]

Hi @bartoszpop , and what about the second option for which I created a PR?

Because I'm not a fan of your solution. Like I explained, I don't think it's a "limitation" at all, it's how other common alias implementations work as well. And given the fact that there is workable solution coming I don't think we need even more flags. So you'd have to convince me that the other option really won't work for you if you want me to change my mind :-)

So this would be a -1 from me for now, but it's @maxandersen that has the final say in these things.

[bartoszpop]

@quintesse as we agreed the order of precedence should be as follows:
user arguments (passed via CLI) --(overrides)--> arguments from jbang-catalog.json --(overrides)--> defaults from the script
My argument is that the second "override" doesn't work because the arguments are put before the Picocli command ("run" in this case), hence are ignored. Using a separate alias seems to work but IMO it is still a workaround and doesn't solve the issue with the second "override" not serving its purpose. Anyway, I appreciate your input and the fix that you provided.

[maxandersen]

it might be too early for me but I'm not fully following the complaint here and why picocli is being mentioned here as the cause/concern ?

[maxandersen]

I'm seeing this referenced from apache/camel#16907 - can we just step back one step and talk about what the actual usecase is here?

The mention about "there is a requirement to have a jbang-catalog.json to be able to override properties inside camel-jbang" for some reason rub me the wrong way. There seem to be some assumptions made on how all this works and I'm not sure which is which ;)

[maxandersen]

I think there is a misunderstanding here ... "arguments" in jbang-catalog.json are arguments for jbang...not for the users app (in this case camel here). we have similar challenge in https://github.com/jbangdev/jbang-action where there are explicit scriptargs and jbangargs for this exact reason.

one thing i've pondered for a while is if we should support the notion of -- <args> which explicitly request the args to be added to the end allowing user to short-circuit the command line.

I think I like that better than #1916 which introduces something that blurrys the lines more IMO.

[bartoszpop]

Hi @maxandersen, thank you for your feedback.

it might be too early for me but I'm not fully following the complaint here and why picocli is being mentioned here as the cause/concern ?

Camel JBang also uses Picocli and the actual script for the camel alias is run --camelArg=... (jbang camel run --camelArg=...), so arguments from jbang-catalog.json are prepended to the script which results in --argFromJbangCatalog=... run --camelArg=... (jbang camel --argFromJbangCatalog=... run --camelArg=...).

we have similar challenge in https://github.com/jbangdev/jbang-action where there are explicit scriptargs and jbangargs for this exact reason.
(...)
I think I like that better than #1916 which introduces something that blurrys the lines more IMO.

I created another pr as per you suggestion.

[quintesse]

"arguments" in jbang-catalog.json are arguments for jbang...not for the users app

Well actually, no, "arguments" are the user arguments, it's a reflection of userParams in the Run command (see https://github.com/jbangdev/jbang/blob/main/src/main/java/dev/jbang/cli/Run.java#L31 and how it connects to arguments here https://github.com/jbangdev/jbang/blob/main/src/main/java/dev/jbang/cli/Run.java#L140)

There is no generic "jbang arguments" field in the alias, all JBang command flags, options and parameters are explicitly mentioned in the alias definition (so if there is a --main option there is also a "main" field, --compile-options --> "compile-options", etc).

[quintesse]

@quintesse thank you for the fix.

Unfortunately I just realized it doesn't fix your issue, so bummer

[quintesse]

So the issue I still have with any "fix" for this, is that it's completely dependent on how you decided to organize your CLI.

You want an alias that makes it easy to run a piece of code, but then you want to insert an extra argument basically at a position of your choosing. You feel that an alias should be able to add extra arguments after the arguments the user typed on the command line, and that this is a completely logical thing to do.

Now, perhaps this is a valid issue and we should just go ahead and accept it (in that case I'd go for the original PR with a `post-arguments" option and no "pre-arguments", I don't see any use for that, that's what "arguments" itself already is). But at the same time I feel it's a weird situation where we need to be fixing other people's design choices.

(Edit: for example, if you wanted to make a Dockerfile with a CMD that executes your Camel command you couldn't do it. You could create a CMD that executes "java -cp ... main.CamelJBang" and user arguments would get added to that. But you can't make it add an extra "--quarkus-version=3.16.0" to the end of the final command line. To me it feels like you're going to the Docker team and asking them to add a POSTCMDARGS option)

[bartoszpop]

I get your point but I still think that arguments from jbang-catalog.json should be appended at the end of the command, although possibly before user provided arguments.

[quintesse]

@bartoszpop hmm I'm not sure if I understand your last remark correctly, because what you're describing is exactly what JBang does, right? THe alias arguments are put before the user arguments:

<command> <alias-arguments> <user-arguments>

which would be:

<... -classpath "(...)" main.CamelJBang> <--quarkus-version=3.16.0> <run * --runtime=quarkus>

which as far as I understood is not what you want.

[bartoszpop]

@quintesse you are correct, I got mixed up (updated the previous comment). Anyway, having the separation for arguments targeted for the jbang command (like in shells) and script arguments seems reasonable to me. I get your point that maybe the script itself should support aliases or predefined arguments but adding scriptArgs (or postArguments) option would cover many use-cases, especially that a similar issue has been already raised for jbang-action.

[quintesse]

especially that a similar issue has been already raised for jbang-action.

Well no, the jbang action is a different use-case. For actions you have only two options, either the arguments are for JBang (which "--quarkus-version" isn't) or the arguments are for the script (which is what the Alias "arguments" are).

But of course in the action your issue doesn't occur because you'd not be mixing pre-defined arguments with user added arguments. You'd always give all necessary arguments and be done with it.

[quintesse]

@bartoszpop Max just realized that there is a possible problem with your use-case: did you think what this issue would do to the other commands that don't support a --quarkus-version option? For example if I type camel log I get:

Unknown option: '--quarkus-version=3.16.0'
Usage: camel log [-h] [--follow] [--logging-color] [--startup] [--timestamp]

[bartoszpop]

True, other commands may fail or ignore an unknown argument depending on the script.

Unknown option: '--quarkus-version=3.16.0'

That's a good point, I haven't thought about this before. Although in our case only "camel run" would be invoked, so it's not a concern, but I got more convinced now not to introduce this parameter.

[quintesse]

But wait, if you're only going to use the alias for "run", why not simply make the alias like this:

      "arguments": ["run", "--quarkus-version=3.16.0"]

Wouldn't that fix your problem?

[bartoszpop]

Yes, for our use case that would work. Thanks @quintesse

[maxandersen]

thanks for staying patient with us @bartoszpop - the issue confused me a bit in start but ultimately starting to add a "append after user argument" will be bad/wrong as it is not reliable. the solution of having explicit alias is much more sensible. I'll consider this issue close for now.

[bartoszpop]

Thank you @maxandersen. Just for completeness, I raised a pr (picocli#2364) to Picocli to allow intercepting the executed commands which will address this issue in a generic manner.

[maxandersen]

I'm perplexed how that Piccoli is going to solve the issue ? You want to be able to modify camel or jbangs picocli parsing here ?

[maxandersen]

I'm perplexed how that Piccoli is going to solve the issue ? You want to be able to modify camel or jbangs picocli parsing here ?

[bartoszpop]

Yes, having a listener in place I would be able to overwrite the default value in the command object.