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

CAMEL-21896: camel-jbang - Allow configuration preset per directory #17593

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Croway
Copy link
Contributor

@Croway Croway commented Mar 27, 2025

Added the following options:

  • --mergeConfigurations loads the configurations default one ~/.camel-jbang-user.properties and the one in the current directory (if exists) ./.camel-jbang-user.properties, the latter is prioritized. This config is a bit hacky, it is used to preload the configuration before the actual picocli execution, if the argument mergeConfigurations exists, the configurations are merged, and the argument removed before passing the args to Picocli.
  • --local Use the configuration in current directory (to be used with camel config commands)

@davsclaus
Copy link
Contributor

I wonder if we need any new options at all. At first glance it seems more complicated and when would you use these options?

I would assume that if there is a local file then its always used and take precedence over the default configuration. That's it.

Are there other CLi tools that has these kind of options and are they also named similar ?

Copy link
Member

@tadayosi tadayosi left a comment

Choose a reason for hiding this comment

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

Thanks, looks great besides the discussion about the usage of --mergeConfigurations. My reply on the topic follows.

@@ -29,13 +29,16 @@ public class ConfigSet extends CamelCommand {
@CommandLine.Parameters(description = "Configuration parameter (ex. key=value)", arity = "1")
String configuration;

@CommandLine.Option(names = { "--local" }, description = "Retrieve configurations from current directory")
Copy link
Member

Choose a reason for hiding this comment

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

The wording should be "Store ... to" instead of "Retrieve ... from" here.

@@ -28,6 +28,9 @@ public class ConfigUnset extends CamelCommand {
@CommandLine.Parameters(description = "Configuration key", arity = "1")
String key;

@CommandLine.Option(names = { "--local" }, description = "Retrieve configurations from current directory")
Copy link
Member

Choose a reason for hiding this comment

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

"Unset" instead of "Retrieve"

@@ -43,13 +43,16 @@ public class VersionSet extends CamelCommand {
@CommandLine.Option(names = { "--reset" }, description = "Reset by removing any custom version settings")
boolean reset;

@CommandLine.Option(names = { "--local" }, description = "Retrieve configurations from current directory")
Copy link
Member

Choose a reason for hiding this comment

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

"Store ... to" over "Retrieve ... from"

Copy link
Member

Choose a reason for hiding this comment

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

Or if we want the same wording for all options, we should consider a more generic wording. Examples:
"Apply configrations to current directoyr", or
"Use local configurations instead of global"?

@tadayosi
Copy link
Member

@davsclaus @Croway

I wonder if we need any new options at all. At first glance it seems more complicated and when would you use these options?

If we start heavily using camel config set/get/list commands, we'll soon want this feature. For example, some camel script might require additional deps such as dependency=dev.langchain4j:langchain4j-ollama, but I'm sure we don't want to apply it to every route. The same should apply to, say, max-idle-seconds=1, and logging-category=org.apache.camel=WARN. Those are not something that should be stored globally, but at the same time I'm sure we don't want to repeat all of these for every run for some specific routes. That's when this new feature works out. You can create a specific working directory, set up a local config, and just run camel run <script>.

I would assume that if there is a local file then its always used and take precedence over the default configuration. That's it.

For --mergeConfigurations, I agree with Claus. it's not critical that we merge local and global configs with this use case. It's not a big deal, we can always use local one or global one. If we want to enable global again, simply rm .camel-jbang-user.properties.

On the other hand, if we accept the new feature, --local should be still required, as otherwise there's no authentic way to store those configs locally. It's not cool that we must first define them with camel config set ... and then manually cp ~/.camel-jbang-user.properties ..

Are there other CLi tools that has these kind of options and are they also named similar ?

direnv uses the source_up instruction to refer to the parent config. Similar function but naming is different.

@davsclaus
Copy link
Contributor

Ah sorry yeah sure the CLI tool for camel config should work for both local and global mode.

@davsclaus
Copy link
Contributor

If the goal is to have per project "camel jbang" configuration file then I think it should be

  • not a hidden file, so its should be named something ala camel-jbang.properties or camel-jbang-local.properties or camel-jbang-user.properties or some other better name.

  • the CLI tool able to modify both local and global options with a --global flag etc.

  • make the local option take precedence (if exists) and fallback to global option (if exist)

  • make the CLI tool able to show a break-down of the options so you can see whether a option is local or global

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

Successfully merging this pull request may close these issues.

4 participants