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

Fix process-compose settings and other process managers #1161

Draft
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

sandydoo
Copy link
Member

@sandydoo sandydoo commented Apr 22, 2024

I started updating process.process-compose to allow overriding individual settings without losing any of the defaults.
But I noticed that, despite the description, those settings are never used.

What needs to be fixed

Name Type Default Comment
process.process-compose attrs { version = "0.5"; ... } Claims to control top-level settings. Only used for tui and unix-socket.
process.process-compose.tui any true Controls the --tui CLI option
process.process-compose.unix-socket any "${config.devenv.runtime}/pc.sock" Sets the --unix-socket path.
process.process-compose.settings yaml {} implementation in config Claims to modify process-specific settings. Controls top-level settings in reality.
process-managers.process-compose.settings.tui bool lib.mkDefault true Never used. Not a PC setting AFAIKT, only command-line.
process-managers.process-compose.settings.port int lib.mkDefault 9999 Never used. Not a PC settings AFAIKT, only command-line.

Other issues

  • tui = false; breaks process-compose. Fixed by serializing the bool to true or false, instead of "1" and "".
  • PC_TUI_ENABLED doesn't exist in process-compose. There's PC_DISABLE_TUI though. edit: nevermind, looks like this exists for backwards-compatibility with previous devenv versions. We should document this. See unable to change process-compose tui #1109.

Proposal

Name Action
process.process-compose Deprecate. If this was previously used for settings, migrate to process-managers.process-compose.settings.
process.process-compose.tui Move to process-managers.process-compose.tui.
process.process-compose.unix-socket Move to process-managers.process-compose.unixSocket. Or unix-socket 🤷.
process-managers.process-compose.settings Fix description to say that this controls the top-level options
process-managers.process-compose.settings.port Move to process-managers.process-compose.port.
process-managers.process-compose.settings.tui Remove. Migrate if previously used.
process-managers.process-compose.useUnixSocket Add. Set to true by default and controls the --use-uds CLI flag. Or enable when port = 0.

@sandydoo sandydoo added the enhancement New feature or request label Apr 22, 2024
@sandydoo
Copy link
Member Author

Relevant example: #1104 (comment)

@@ -70,7 +70,7 @@ in
default = true;
};
unix-socket = lib.mkOption {
type = types.str;
type = types.str; # TODO: should be path?
Copy link
Contributor

Choose a reason for hiding this comment

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

Paths get copied to the /nix/store. Most of the time you don't want that, especially for devenv runtime state.

Copy link
Member Author

Choose a reason for hiding this comment

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

TIL! Nice

@domenkozar
Copy link
Member

domenkozar commented Apr 23, 2024

We also should update examples and docs and make sure all options are deprecated/renamed.

Copy link

cloudflare-workers-and-pages bot commented Sep 2, 2024

Deploying devenv with  Cloudflare Pages  Cloudflare Pages

Latest commit: 0743787
Status: ✅  Deploy successful!
Preview URL: https://c760abfb.devenv.pages.dev
Branch Preview URL: https://process-compose-settings.devenv.pages.dev

View logs

@sandydoo sandydoo changed the title draft: attempt to unify process-compose settings Fix process-compose settings and other process managers Sep 3, 2024
@sandydoo
Copy link
Member Author

sandydoo commented Sep 3, 2024

An update on the changes so far.

Following the same pattern, we were thinking of renaming process-managers.<name>.* to process.managers.<name>.*. I'm not entirely sure about this one yet.

Process manager changes

Before After Comment
process.implementation process.manager.implementation
process.after process.manager.after
process.before process.manager.before
processManagerCommand process.manager.command
process.manager.args Allows modifying the options passed to the command.
process-managers.<name>.enable Changed to internal = true. These are set via process.manager.implementation. Added an assertion to check that only one is enabled.

Process-compose changes

Before After Comment
process.process-compose.tui process-managers.process-compose.tui.enable
process-managers.process-compose.unixSocket.enable Enables the unix socket. Default: true.
process.process-compose.unix-socket process-managers.process-compose.unixSocket.path
process.process-compose.* Rest dropped. Never used anywhere.
process-managers.process-compose.settings.port process-managers.process-compose.port

@domenkozar
Copy link
Member

Following the same pattern, we were thinking of renaming process-managers..* to process.managers..*. I'm not entirely sure about this one yet.

This follows the same pattern as container.* and containers.<name>.*, same for processes, etc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants