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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

draft: attempt to unify process-compose settings #1161

Draft
wants to merge 3 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.

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.

None yet

3 participants