refactor: cleanup and refactor code #10

Merged

kmendell merged 1 commit from refactor/cleanup-code-v1 into main 2026-10-13 12:52:11 -05:00

Conversation 0 Commits 1 Files changed 238 +6081 -11717

kmendell commented 2026-10-13 12:24:06 -05:00

Owner

• Copy link
• Copy Markdown

What changed

• No Internal suffixes remain. Small helpers with one or two callers are inlined. Signatures that were split across lines are joined, except four with long parameter lists that are now wrapped normally.
• Main duplicate merges:
  • Config validation: the five "validate items, reject duplicate names" loops in internal/config are now one helper.
  • Confstore merges: five "merge config-dir entries over main-file entries by name" copies in confstore are now mergeByName.
  • Store: the bootstrap-claim checks, the user transaction wrapper and the desktop-endpoint upserts are shared.
  • authz: its hand-written label matching now uses labels.Match.
  • Certificates: the three Sign*Cert functions in ca now build their common fields in one place.
  • Web: JSON request decoding, the session cookie, desktop node authorization, probes, Proxmox guest lookup, and the node login check in the terminal and file handlers are each shared.
  • Clients: Connect/ConnectDesktop in the client share one path, and the five proxy tunnel-rejection audit calls are one.
• Kept on purpose:
  • sliceutil.Dedup stays: unlike kit.Unique, it returns an empty list rather than nil, which matters for JSON output.
  • EnsureNodeDesktopEndpoint keeps its own SQL because it deliberately doesn't overwrite the Proxmox columns.
  • Some one- or two-caller helpers stay where inlining would push a function over the complexity limit of 40, or where they have real table tests.
• Lint fixes: I put var installService = winsvc.Install back in cmd/overpass/main.go. Without it, staticcheck flags the install call off Windows (that build's Install always returns an error). I also cleared five reused-err warnings and formatted two files.

Behaviour changes to review

• Web login and change-password now reject unknown JSON fields; only the bundled UI calls them. CLI and agent endpoints still accept unknown fields.
• Proxmox shell:
  • Opening a shell on a template now returns 409 "templates have no shell", which is a new message.
  • The pve.shell.revoked audit event now records the real node instead of the provider name.
  • Proxmox probe log lines now say "desktop probe failed".
• Client setup now returns download_base_url with the trailing / trimmed.
• Audit: an until of exactly the zero time now means "no limit".
• Errors now listed or reported differently:
  • Unknown connector roles are now all listed, sorted (unknown role: a, b), instead of only the first.
  • An empty name for a desktop credential, desktop endpoint or Proxmox provider in YAML now includes its position ("desktop credential 0: name required"), matching roles and OIDC.
  • The two "not a certificate" messages are merged into one, "credential is not a certificate".
  • A profile-key failure in Connect now says "reading profile key: …" instead of "loading certificate: …".
• OIDC:
  • Role names from claim mappings are trimmed, and empty ones are dropped.
  • A mapping with an empty value no longer matches an empty claim.
  • client_secret_env is no longer copied into the stored config; the resolved connector still gets the secret.
• Node:
  • login.defs and /etc/paths* are read whole, so a read error now gives no entries instead of the part already read.
  • findTool now only accepts regular files.

## What changed - **No `Internal` suffixes remain.** Small helpers with one or two callers are inlined. Signatures that were split across lines are joined, except four with long parameter lists that are now wrapped normally. - **Main duplicate merges:** - **Config validation:** the five "validate items, reject duplicate names" loops in `internal/config` are now one helper. - **Confstore merges:** five "merge config-dir entries over main-file entries by name" copies in `confstore` are now `mergeByName`. - **Store:** the bootstrap-claim checks, the user transaction wrapper and the desktop-endpoint upserts are shared. - **authz:** its hand-written label matching now uses `labels.Match`. - **Certificates:** the three `Sign*Cert` functions in `ca` now build their common fields in one place. - **Web:** JSON request decoding, the session cookie, desktop node authorization, probes, Proxmox guest lookup, and the node login check in the terminal and file handlers are each shared. - **Clients:** `Connect`/`ConnectDesktop` in the client share one path, and the five proxy tunnel-rejection audit calls are one. - **Kept on purpose:** - `sliceutil.Dedup` stays: unlike `kit.Unique`, it returns an empty list rather than nil, which matters for JSON output. - `EnsureNodeDesktopEndpoint` keeps its own SQL because it deliberately doesn't overwrite the Proxmox columns. - Some one- or two-caller helpers stay where inlining would push a function over the complexity limit of 40, or where they have real table tests. - **Lint fixes:** I put `var installService = winsvc.Install` back in `cmd/overpass/main.go`. Without it, staticcheck flags the install call off Windows (that build's `Install` always returns an error). I also cleared five reused-`err` warnings and formatted two files. ## Behaviour changes to review - **Web login and change-password** now reject unknown JSON fields; only the bundled UI calls them. CLI and agent endpoints still accept unknown fields. - **Proxmox shell:** - Opening a shell on a template now returns 409 "templates have no shell", which is a new message. - The `pve.shell.revoked` audit event now records the real node instead of the provider name. - Proxmox probe log lines now say "desktop probe failed". - **Client setup** now returns `download_base_url` with the trailing `/` trimmed. - **Audit:** an `until` of exactly the zero time now means "no limit". - **Errors now listed or reported differently:** - Unknown connector roles are now all listed, sorted (`unknown role: a, b`), instead of only the first. - An empty name for a desktop credential, desktop endpoint or Proxmox provider in YAML now includes its position ("desktop credential 0: name required"), matching roles and OIDC. - The two "not a certificate" messages are merged into one, "credential is not a certificate". - A profile-key failure in `Connect` now says "reading profile key: …" instead of "loading certificate: …". - **OIDC:** - Role names from claim mappings are trimmed, and empty ones are dropped. - A mapping with an empty value no longer matches an empty claim. - `client_secret_env` is no longer copied into the stored config; the resolved connector still gets the secret. - **Node:** - `login.defs` and `/etc/paths*` are read whole, so a read error now gives no entries instead of the part already read. - `findTool` now only accepts regular files.

kmendell added 1 commit 2026-10-13 12:24:06 -05:00

refactor: cleanup and refactor code 2b37d8b62e

kmendell force-pushed refactor/cleanup-code-v1 from 2b37d8b62e to 7b55dd7e00 2026-10-13 12:39:36 -05:00 Compare

kmendell force-pushed refactor/cleanup-code-v1 from 7b55dd7e00 to be8fbfd1ce 2026-10-13 12:43:39 -05:00 Compare

kmendell force-pushed refactor/cleanup-code-v1 from be8fbfd1ce to 9f3115a064 2026-10-13 12:49:40 -05:00 Compare

kmendell merged commit f5efd753c3 into main 2026-10-13 12:52:11 -05:00

kmendell deleted branch refactor/cleanup-code-v1 2026-10-13 12:52:11 -05:00

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

ofkm/overpass!10

No description provided.