make arguments configurable via options #81

VanCoding · 2024-10-01T07:09:50Z

Here I've made options for every process-compose argument there currently is.
This makes it nicer to set arguments in a modular way, and also paves the way to conditionally pass arguments to process-compose based on the subcommand (don't pass --config to the attach or downcommand for example)

For compatibility, I've remapped the tui and httpServer options to set these new options under the hood.

Also worth mentioning: The "normal" and the "test"-process use different arguments, to solve this problem I made separate "test-arguments" options, that default to the normal options.

srid

Useful addition! Here's my initial review.

nix/process-compose-flake-lib.nix

srid · 2024-10-01T16:50:04Z

nix/lib.nix

@@ -39,6 +39,7 @@ rec {
  evalModules = { name ? "process-compose", modules }: (lib.evalModules {
    specialArgs = {
      inherit name pkgs;
+      process-compose-flake-lib = (import ./process-compose-flake-lib.nix) { lib = pkgs.lib; };


I'd expect a special arg like process-compose-flake-lib to point to ./nix/lib.nix. Besides, as commented elsewhere, a function like mkProcessComposeArgumentsOption seems redundant (unless I'm missing something), and that you can simply import that submodule directly for re-use.

srid · 2024-10-01T16:51:21Z

nix/process-compose-flake-lib.nix

+              };
+              sort = mkOption {
+                type = types.str;
+                default = "";


Instead of using empty strings (which has ambiguous meaning, since you are forced to look at the implementation to understand the semantics), we should use types.nullOr types.str as type in options like this.

I've used nullable strings in the past as well, but don't do that nowadays, except if null and "" has different meaning. In this case, it doesn't. The string being nullable only adds the burden of needing to check if it's null in addition to needing to check if it's empty. Same goes for arrays, i always default to [] instead of null for the same reasons.

But in the end it's a matter of taste and I'll follow your command.

But in the end it's a matter of taste and I'll follow your command.

Ok.

srid · 2024-10-01T16:52:17Z

nix/process-compose/cli.nix

@@ -70,6 +57,22 @@ in
        default = true;
        description = "Enable or disable the TUI for the application.";
      };
+      arguments = process-compose-flake-lib.mkProcessComposeArgumentsOption { };


cli is simpler I think.

Suggested change

arguments = process-compose-flake-lib.mkProcessComposeArgumentsOption { };

cli = process-compose-flake-lib.mkProcessComposeArgumentsOption { };

srid · 2024-10-01T16:53:28Z

nix/process-compose/cli.nix

@@ -70,6 +57,22 @@ in
        default = true;
        description = "Enable or disable the TUI for the application.";
      };
+      arguments = process-compose-flake-lib.mkProcessComposeArgumentsOption { };
+      test-arguments = process-compose-flake-lib.mkProcessComposeArgumentsOption { };


Do we expect users to be able to set test-arguments?

I don't know, I mainly introduced this to be able to configure the "config" argument in the same way as every other argument. And because this one depends on it being a test-process or not, I couldn't come up with another solution.

But I mean.. why not? Could be handy, no?

nix/process-compose/default.nix

VanCoding · 2024-10-02T19:27:59Z

@srid I've done the refactoring as requested, except of removing "test-arguments" (now test-cli). Additionally, I've made an optional commit which moved actually building the CLI arguments into output options, but I'm not sure if that's a good idea. I'd revert it if it was my decision :D

srid

Final review, hopefully!

srid · 2024-10-05T21:58:23Z

nix/process-compose/cli.nix

+        up = (lib.removeAttrs config.cli.up [ "output" ]) // { config = [ "${config.outputs.settingsTestFile}" ]; };
+        global = lib.removeAttrs config.cli.global [ "output" ];


Why is output being removed? Can you add a comment explaining the purpose of this logic?

It took me a while to figure this out as well. If we don't remove it we'resetting the output of test-cli to the output of the normal cli, which completely misses the point.

srid · 2024-10-05T21:58:40Z

nix/process-compose/cli.nix

+  config = {
+    cli = lib.mkMerge [{
+      up = {
+        config = [ "${config.outputs.settingsFile}" ];
+        tui = config.tui;
+      };
+      global = {
+        port = config.httpServer.port;
+        use-uds = config.httpServer.uds != false;
+        unix-socket = if builtins.isString config.httpServer.uds then config.httpServer.uds else "";
+        no-server = if config.httpServer.enable == true then false else true;
+      };
+    }];
+    test-cli = lib.mkMerge [
+      {
+        up = (lib.removeAttrs config.cli.up [ "output" ]) // { config = [ "${config.outputs.settingsTestFile}" ]; };
+        global = lib.removeAttrs config.cli.global [ "output" ];
+      }
+    ];
+  };


You don't need mkMerge with singleton lists.

I don't know, i'd need to test it again, but with my tests it was needed. But don't ask me why, i'm no expert of how the module system works.

nix/process-compose/cli.nix

nix/process-compose/cli-options.nix

nix/process-compose/default.nix

VanCoding · 2024-10-05T23:28:18Z

Maybe I'll have the energy to go all over it once again, but this is already much more cumbersome than i initially thought, so i'll prioritize it a bit less.

…ptions and handle it manually

VanCoding · 2024-10-06T09:49:31Z

@srid I've done another round of refactoring. I think this should account for all of your comments except for the "up" subcommand.

Sorry for the slightly negative tone of the previous command btw, I was a bit frustrated. But I'm learning a lot while doing this so it's fine :)

srid · 2024-10-14T00:39:38Z

@VanCoding No worries (I didn't sense any negative tone).

Sorry I've let this PR slip by. It generally looks good to me, but I'll check it locally and give it a thorough review, push some commits and then squash merge.

Because we use types.nullOr

Yes, this will require users to migrate that's fine.

srid

I'm happy with the PR, the diff is small and it does one thing (implement most of the CLI options of process-compose, encapsulated nicely in a submodule) well.

VanCoding · 2024-10-14T14:15:07Z

@srid Nice! Thanks for merging that one in.

Regarding the subcommand topic: This is a really tricky one, and I've already had a hacky solution here: VanCoding@5b66d08

But that's also not perfect, because then the subcommand has to be the first argument. And properly parsing the arguments just for this is probably overkill, so I don't know what's the best way to approach this. I'm alreay happy if it's possible to use subcommands other than "up", which with the current solution is the case.

If it's okay for you I'll open another PR to change the ponysay example to use the "detached" mode, and add the comments as in the referenced commit above.

Culprit seems to be Platonic-Systems/process-compose-flake#81

Culprit seems to be Platonic-Systems/process-compose-flake#81 Flake lock file updates: • Updated input 'process-compose-flake': 'github:Platonic-systems/process-compose-flake/29301aec92d73c9b075fcfd06a6fb18665bfe6b5?narHash=sha256-yEMzxZfy%2BEE9gSqn%2B%2BSyZeAVHXYupFT8Wyf99Z/CXXU%3D' (2024-10-14) → 'github:Platonic-systems/process-compose-flake/40a3d1b892e4669ac23d215ef1ab5db6e6a63136?narHash=sha256-dZHk%2BQNBTpg5V6dEXcP4paOyNCDq8YTmNjb0ICh3FdQ%3D' (2024-10-14)

roberth · 2024-10-18T10:30:43Z

This adds many undocumented options. Please add documentation so we can publish the docs on flake.parts.
It'd be good to have a CI check as well, although I'm surprised that this passed review without documentation.

VanCoding · 2024-10-18T12:11:27Z

@roberth Yeah, it wasn't originally meant to be a breaking change.

Fortunately, the new options closely reflect the cli arguments of process-compose , so if you know these, you also know the options.

But it's of course even better to have these documented with all the other options, so let me know if i can help with that.

roberth · 2024-10-18T14:17:18Z

Links to the upstream docs would do the job just fine.

wasn't originally meant to be a breaking change.

Of course. Sorry for the grumpy mood. This looks like a nice addition actually :)

srid · 2024-10-18T18:32:05Z

It'd be good to have a CI check as well,

@roberth Is there something in upstream that I can use easily in downstream module? Something as easy as say import'ing the flake-parts module.

In this particular case, though, these options need not strictly be documented due to the 1-on-1 map with upstream. However, when refactoring it¹, we could add some stub docs to make flake-parts docs site check happy.

https://github.com/Platonic-Systems/process-compose-flake/blob/29301aec92d73c9b075fcfd06a6fb18665bfe6b5/nix/process-compose/cli-options.nix#L91 ↩

roberth · 2024-10-19T20:40:00Z

need not strictly be documented due to the 1-on-1 map with upstream.

That's not the excuse, but the thing to document.

Is there something in upstream that I can use easily in downstream module?

nix build github:hercules-ci/flake.parts-website --override-input process-compose-flake .

say import'ing the flake-parts module.

Not feasible until NixOS/nix#7730 is implemented, except perhaps with workarounds that isolate the dependencies.
Personally I'm sick of working around that, so I'm ok with the nix build command for the time being.

roberth · 2024-10-19T20:41:32Z

Even better:

nix build github:hercules-ci/flake.parts-website#checks.x86_64-linux.linkcheck --override-input process-compose-flake .

The link check is very fast nowadays, thanks to the lychee project.

srid · 2024-10-19T20:44:59Z

That's not reproducible. Even when I make it reproducible (srid/haskell-flake#244) it fails on when next updating flake-parts-website (why srid/haskell-flake#354 is never merged).

srid · 2024-10-19T21:17:05Z

That's not the excuse, but the thing to document.

I'm not opposed to adding placeholder description to these options, but I cannot guarantee that future PRs will stick to consistent options documentation without a reliable CI check in place.

Also, it would be helpful to others to document these expectations when they are adding their module to https://github.com/hercules-ci/flake.parts-website

I'm ok with the nix build command for the time being.

I'm not comfortable with using non-reproducible commands in CI, though. #81 (comment)

roberth · 2024-10-19T21:18:18Z

I don't see how that wouldn't be reproducible, because everything seems to be pinned in doc/flake.lock.

That said, I wouldn't be surprised if Nix still had a bug with input overriding, and that would be another reason to implement NixOS/nix#7730. Overriding is more natural to implement in the expression language, following the usual implementation patterns for that (ie lib.fixedPoints, without actual lib).

srid · 2024-10-21T17:14:48Z

I've added description to all options and added the linkCheck to CI (if the check fails in future for reasons outside of the scope of this repo, I may have to remove it)

#84

014bfa1

roberth · 2024-10-21T20:44:32Z

They're live again.

may have to remove it

Of course you'll be free to do so, and conversely I may have to pin it again, but until either of that happens, progress will be smoother. Thanks for adding the check!

VanCoding force-pushed the argument-options branch from 82f98b1 to b0772ec Compare October 1, 2024 07:20

make arguments configurable via options

86bc80b

VanCoding force-pushed the argument-options branch from b0772ec to 86bc80b Compare October 1, 2024 08:17

fix getting up& global args

573d5d4

srid reviewed Oct 1, 2024

View reviewed changes

VanCoding added 2 commits October 2, 2024 21:02

refactor accoring to comments

c02f44b

cli output options (optional)

7286df6

srid requested changes Oct 5, 2024

View reviewed changes

refactor cli into its own module, remove "test-cli" and "up.config" o…

4b2695f

…ptions and handle it manually

VanCoding added 2 commits October 6, 2024 16:06

optimize diff

2a92663

remove dead code

449a605

srid and others added 8 commits October 13, 2024 20:44

Merge branch 'main' into argument-options

4e69345

Use new cli options in example

bea8e2f

Follow consistent naming convention, such as outputs.settingsFile

710c376

Fix argument handling behaviour

57bf61a

Remove redundant empty string comparison

868fefe

Because we use types.nullOr

let's refactor in future

27cf184

Get rid of legacy options, in favour of the new 'cli' submodule

aaabc87

Yes, this will require users to migrate that's fine.

changelog

375e79a

srid approved these changes Oct 14, 2024

View reviewed changes

add breaking change note

e502b37

srid merged commit 4448f64 into Platonic-Systems:main Oct 14, 2024
2 checks passed

VanCoding mentioned this pull request Oct 14, 2024

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

Closed

roberth added a commit to hercules-ci/flake.parts-website that referenced this pull request Oct 18, 2024

pin process-compose while project has undocumented options

c038a06

Culprit seems to be Platonic-Systems/process-compose-flake#81

srid mentioned this pull request Oct 21, 2024

Restore old CLI behaviour, while retaining global options added recently #84

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

make arguments configurable via options #81

make arguments configurable via options #81

VanCoding commented Oct 1, 2024 •

edited

Loading

srid left a comment

srid Oct 1, 2024

srid Oct 1, 2024

VanCoding Oct 1, 2024

srid Oct 2, 2024

srid Oct 1, 2024

srid Oct 1, 2024

VanCoding Oct 1, 2024

VanCoding commented Oct 2, 2024

srid left a comment

srid Oct 5, 2024

VanCoding Oct 5, 2024

srid Oct 5, 2024

VanCoding Oct 5, 2024

VanCoding commented Oct 5, 2024

VanCoding commented Oct 6, 2024 •

edited

Loading

srid commented Oct 14, 2024

srid left a comment

VanCoding commented Oct 14, 2024

roberth commented Oct 18, 2024

VanCoding commented Oct 18, 2024

roberth commented Oct 18, 2024

srid commented Oct 18, 2024

roberth commented Oct 19, 2024

roberth commented Oct 19, 2024

srid commented Oct 19, 2024

srid commented Oct 19, 2024

roberth commented Oct 19, 2024

srid commented Oct 21, 2024 •

edited

Loading

roberth commented Oct 21, 2024

	arguments = process-compose-flake-lib.mkProcessComposeArgumentsOption { };
	cli = process-compose-flake-lib.mkProcessComposeArgumentsOption { };

		up = (lib.removeAttrs config.cli.up [ "output" ]) // { config = [ "${config.outputs.settingsTestFile}" ]; };
		global = lib.removeAttrs config.cli.global [ "output" ];

make arguments configurable via options #81

make arguments configurable via options #81

Conversation

VanCoding commented Oct 1, 2024 • edited Loading

srid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VanCoding commented Oct 2, 2024

srid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VanCoding commented Oct 5, 2024

VanCoding commented Oct 6, 2024 • edited Loading

srid commented Oct 14, 2024

srid left a comment

Choose a reason for hiding this comment

VanCoding commented Oct 14, 2024

roberth commented Oct 18, 2024

VanCoding commented Oct 18, 2024

roberth commented Oct 18, 2024

srid commented Oct 18, 2024

Footnotes

roberth commented Oct 19, 2024

roberth commented Oct 19, 2024

srid commented Oct 19, 2024

srid commented Oct 19, 2024

roberth commented Oct 19, 2024

srid commented Oct 21, 2024 • edited Loading

roberth commented Oct 21, 2024

VanCoding commented Oct 1, 2024 •

edited

Loading

VanCoding commented Oct 6, 2024 •

edited

Loading

srid commented Oct 21, 2024 •

edited

Loading