Skip to content

feat: add Default config which applies to all servers - #61

Merged
mikew merged 4 commits into
mikew:mainfrom
sevenc-nanashi:feat/default-server-config
Sep 30, 2025
Merged

feat: add Default config which applies to all servers#61
mikew merged 4 commits into
mikew:mainfrom
sevenc-nanashi:feat/default-server-config

Conversation

@sevenc-nanashi

@sevenc-nanashi sevenc-nanashi commented Sep 30, 2025

Copy link
Copy Markdown
Contributor

This PR adds Default config which applies to all servers, with some extra configuration such as local-editor.

This PR closes #60 .
The configuration file will look like:

default:
  local-editor:
    [
      "C:\\Program Files\\Neovide\\neovide.exe",
      "--no-fork",
      "--server={{SOCKET_PATH}}",
    ]

servers:
  myServer1:
    ...
  myServer2:
    local-editor: ...

Comment thread src/nvrh_config/main.go Outdated
NvimCmd []string `yaml:"nvim-cmd,omitempty"`
UsePorts *bool `yaml:"use-ports,omitempty"`
SshArg []string `yaml:"ssh-arg,omitempty"`
SshPath string `yaml:"ssh-path,omitempty"`

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Given where ApplyPrecedence is called in the flow, getSshPath isn't applied to it. So in yaml ssh-path: binary would look for a literal executable named binary in your $PATH, and not actually use the default.

Comment thread src/nvrh_config/main.go Outdated
SshArg []string `yaml:"ssh-arg,omitempty"`
SshPath string `yaml:"ssh-path,omitempty"`
LocalEditor []string `yaml:"local-editor,omitempty"`
ServerEnv map[string]string `yaml:"server-env,omitempty"`

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Would prefer if this was just []string:

  • via cli arg, they're FOO=bar
  • via env vars, they're FOO=bar
  • via yaml, they're FOO: bar

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Ah, more changes are needed to pipe these through is the issue.

Comment thread src/nvrh_config/main.go
return err
}

// Fall back to environment variables if still not set.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Huh, I can see that was was just moved to DRY the yaml -> CLI stuff, so I might have had a bug here. I have $NVRH_CLIENT_SERVER_ENV set in my environment and that's taking precedence over anything in the YAML.

Comment thread src/nvrh_config/main.go Outdated
SshArg []string `yaml:"ssh-arg,omitempty"`
SshPath string `yaml:"ssh-path,omitempty"`
LocalEditor []string `yaml:"local-editor,omitempty"`
ServerEnv map[string]string `yaml:"server-env,omitempty"`

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Ah, more changes are needed to pipe these through is the issue.

Comment thread src/nvrh_config/main.go
}

var envIndex = map[string][]string{
"ssh-path": {"NVRH_CLIENT_SSH_PATH"},

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

When adding to this, we need to remove the source from the CLI command, similar to

nvrh/src/client/main.go

Lines 67 to 72 in 720d3d7

&cli.BoolFlag{
Name: "use-ports",
Usage: "Use ports instead of sockets. Defaults to true on Windows [$NVRH_CLIENT_USE_PORTS]",
// Sources: cli.EnvVars("NVRH_CLIENT_USE_PORTS"),
Value: runtime.GOOS == "windows",
},

@mikew

mikew commented Sep 30, 2025

Copy link
Copy Markdown
Owner

I went ahead and made those changes, this works a treat, thanks for the PR!

@mikew
mikew merged commit ab63473 into mikew:main Sep 30, 2025
3 checks passed
mikew pushed a commit that referenced this pull request Oct 3, 2025
🤖 I have created a release *beep* *boop*
---


## [0.5.0](v0.4.0...v0.5.0)
(2025-10-01)


### Features

* add Default config which applies to all servers
([#61](#61))
([ab63473](ab63473))
* replace remote directory's leading tilde with HOME variable
([#64](#64))
([e4c2710](e4c2710))


### Bug Fixes

* Add pull_request trigger to ci
([ccfce59](ccfce59))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Default config

2 participants