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.

Sign up for GitHub

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

allow "attach" and "down" subcommands to work, show it in ponysay example #82

Closed
wants to merge 5 commits into from
Closed

allow "attach" and "down" subcommands to work, show it in ponysay example #82

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1c6fa84
allow "attach" and "down" subcommands to work, show it in ponysay exa…
VanCoding Oct 14, 2024
4e42448
detect "up" subommand and add arguments accordingly
VanCoding Oct 14, 2024
c0fb6ab
change order of attributes
VanCoding Oct 14, 2024
58d976e
require passing a subcommand as the first argument, fail with instruc…
VanCoding Oct 20, 2024
25804f1
Revert "require passing a subcommand as the first argument, fail with…
VanCoding Oct 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions example/flake.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,8 +88,11 @@
};

# nix run .#ponysay up to start the process
srid marked this conversation as resolved.
Show resolved Hide resolved
# nun run .#ponysay attach to show the output
# nix run .#ponysay down to stop the process
packages.ponysay = (import inputs.process-compose-flake.lib { inherit pkgs; }).makeProcessCompose {
modules = [{
cli.up.detached = true;
srid marked this conversation as resolved.
Show resolved Hide resolved
settings = {
processes = {
ponysay.command = ''
Expand Down
20 changes: 12 additions & 8 deletions nix/process-compose/default.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,17 +45,21 @@ in
text = ''
${preHook}

run-process-compose () {
set -x; process-compose ${cliOutputs.global} --config ${configFile} "$@"; set +x
}

# Run `up` command, with arguments; unless the user wants to pass their own subcommand.
if [ "$#" -eq 0 ]; then
run-process-compose up ${cliOutputs.up}
else
run-process-compose "$@"
# If there are no arguments, it's the "up" command
# If the first argument is "up", it's also the "up" command
# If the first argument starts with a dash, we assume there isn't a subcommand, so it's also the "up" command
# Otherwise, we assume it's a subcommand other than "up"
params=(${cliOutputs.global})
set +u
if [ -z "$1" ] || [[ "$1" == "up" ]] || [[ "$1" == -* ]] ; then
Copy link
Member

Choose a reason for hiding this comment

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

I don't see what purpose [[ "$1" == -* ]] serves here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to explain it in the comments, but it looks like it failed 😅

If the first argument starts with a dash, we know it's no subcommand. And if the first argument is no subcommand, we assume there is no subcommand argument at all. And if there is none, process-compose defaults to the up command. And therefore it's allowed to pass arguments dedicated to the up command.

Copy link
Member

@srid srid Oct 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I understood the what, but I'm asking why.

If the first argument starts with a dash, we assume there isn't a subcommand, so it's also the "up" command

This is of course wrong, since --help is not equivalent to up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that seems to be one of the edge cases I mentioned. Properly dealing with it could be tricky, though. Any ideas?

Copy link
Member

Choose a reason for hiding this comment

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

Why not remove [[ "$1" == -* ]] entirely?

params+=(--config ${configFile} ${cliOutputs.up})
Copy link
Member

Choose a reason for hiding this comment

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

In the "$1" == "up" case, shouldn't you drop the up from $@? Otherwise, in line 60 it will be passed twice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, we're passing the arguments on as we receive them, and we never add an "up" ourselves afaik

Copy link
Member

@srid srid Oct 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

You are right:

❮ nix run --override-input process-compose-flake .. .#default -- up --log-file=output.log
[...]
+ process-compose --no-server --config /nix/store/bl46rll1p24g5fbdypbxsk6rfnklfp2z-process-compose-default.json --disable-dotenv --theme Cobalt up --log-file=output.log

But then, I just noticed something new here. Note that --disable-dotenv --theme Cobalt (defined in cli.up) appears before the up argument. It looks like their CLI parser accepts this out of order arguments anyway. But for clarity, I'd prefer that --disable-dotenv --theme Cobalt appear after the up argument, because those options are meant for up subcommand anyway.

Suppressing incorrect observation Wait, I was wrong. Actually those are global options! We should fix this in options. image

--sort however is an up specific option, so that's a better illustration is another example:

--- a/example/flake.nix
+++ b/example/flake.nix
@@ -30,8 +30,7 @@
               };
               # Options for `process-compose up`
               up = {
-                disable-dotenv = true;
-                theme = "Cobalt";
+                sort = "health";
               };
             };
             settings = {

Without arguments, we see:

❯ nix run --override-input process-compose-flake .. .#default
[..]
+ process-compose --no-server --config /nix/store/bl46rll1p24g5fbdypbxsk6rfnklfp2z-process-compose-default.json --sort health

(And I'd still like to make up implicit here)

With up argument:

❯ nix run --override-input process-compose-flake .. .#default up
+ process-compose --no-server --config /nix/store/bl46rll1p24g5fbdypbxsk6rfnklfp2z-process-compose-default.json --sort health up

Here, I think --sort health should appear after up to make it clear (to the user who happens to diagnose our script output) that these flags are for up command only.

This comment was marked as resolved.

Sign in to view

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, it's better to put user arguments first. I've changed that.

fi

set -x
process-compose "''${params[@]}" "$@"
set +x

${postHook}
'';
};
Expand Down
2 changes: 1 addition & 1 deletion nix/process-compose/test.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ in
export HOME=$TMP
cd $HOME
# Run with tui disabled because /dev/tty is disabled in the simulated shell
${lib.getExe config.outputs.testPackage} -t=false
${lib.getExe config.outputs.testPackage} up -t=false
# `runCommand` will fail if $out isn't created
touch $out
''
Expand Down