Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/).
- Container image now runs `serve` by default and loads bundled config from `$KO_DATA_PATH`
- `PKGPROXY_TRUST_PROXY` env var (and `--trust-proxy` flag) to opt in to X-Forwarded-For trust
- `PKGPROXY_HOST` env var to set the listen address without passing `--host` on the command line
- `PKGPROXY_CACHEDIR` env var to set the cache directory without passing `--cachedir` on the command line

### Changed

Expand Down
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ podman run --rm -p 8080:8080 -e PKGPROXY_HOST=0.0.0.0 --volume ./cache:/ko-app/c
| Flag | Env Variable | Default | Description |
|------|--------------|---------|-------------|
| `--config, -c` | `PKGPROXY_CONFIG` | `./pkgproxy.yaml` | Path to the repository config file |
| `--cachedir` | | `cache` | Path to the local cache directory |
| `--cachedir` | `PKGPROXY_CACHEDIR` | `cache` | Path to the local cache directory |
| `--host` | `PKGPROXY_HOST` | `localhost` | Listen address |
| `--port` | | `8080` | Listen port |
| `--public-host` | `PKGPROXY_PUBLIC_HOST` | | Public hostname (or `host:port`) shown in landing page config snippets. When set, the listen port is not appended. Useful when running behind a reverse proxy. |
Expand Down
13 changes: 13 additions & 0 deletions cmd/root.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,24 @@ var (
)

const (
cachedirEnvVar = "PKGPROXY_CACHEDIR"
configPathEnvVar = "PKGPROXY_CONFIG"
defaultConfigPath = "./pkgproxy.yaml"
defaultDir = "cache"
)

// resolveCacheDir determines the local cache directory using flag → env var →
// default precedence.
func resolveCacheDir(flagChanged bool, flagValue, envValue string) string {
if flagChanged {
return flagValue
}
if envValue != "" {
return envValue
}
return defaultDir
}

// NewRootCommand creates a new root cli command instance
func NewRootCommand() *cobra.Command {
c := &cobra.Command{
Expand Down
1 change: 1 addition & 0 deletions cmd/serve.go
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ func newServeCommand() *cobra.Command {
Args: cobra.ArbitraryArgs,
Short: "Start forward proxy",
PersistentPreRunE: func(cmd *cobra.Command, _ []string) error {
cacheDir = resolveCacheDir(cmd.Flag("cachedir").Changed, cacheDir, os.Getenv(cachedirEnvVar))
listenAddress = resolveListenHost(cmd.Flag("host").Changed, listenAddress, os.Getenv(hostEnvVar))
resolvedTrustProxy = resolveTrustProxy(cmd.Flag("trust-proxy").Changed, trustProxy, os.Getenv(trustProxyEnvVar))
var err error
Expand Down
52 changes: 52 additions & 0 deletions cmd/serve_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,58 @@ func TestResolveListenHost(t *testing.T) {
}
}

func TestResolveCacheDir(t *testing.T) {
tests := []struct {
name string
flagChanged bool
flagValue string
envValue string
want string
}{
{
name: "flag changed wins over env var",
flagChanged: true,
flagValue: "/data/cache",
envValue: "/other",
want: "/data/cache",
},
{
name: "flag changed wins even when value equals default",
flagChanged: true,
flagValue: "cache",
envValue: "/data/cache",
want: "cache",
},
{
name: "env var used when flag unchanged",
flagChanged: false,
flagValue: "cache",
envValue: "/var/cache/pkgproxy",
want: "/var/cache/pkgproxy",
},
{
name: "empty env var falls through to default",
flagChanged: false,
flagValue: "cache",
envValue: "",
want: "cache",
},
{
name: "neither set returns default",
flagChanged: false,
flagValue: "cache",
envValue: "",
want: "cache",
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
got := resolveCacheDir(tt.flagChanged, tt.flagValue, tt.envValue)
assert.Equal(t, tt.want, got)
})
}
}

func TestResolveTrustProxy(t *testing.T) {
tests := []struct {
name string
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
schema: spec-driven
created: 2026-08-28
91 changes: 91 additions & 0 deletions openspec/changes/archive/2026-08-28-cachedir-env-var/design.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
## Context

`--cachedir` is a persistent flag declared on the **root** command (`cmd/root.go:43`), defaulting
to `cache`. Only the `serve` subcommand reads the resolved value — `startServer` passes `cacheDir`
into `newEchoApp` (`cmd/serve.go:239`). No env var currently influences it.

The project has three flag↔env precedents, split across two patterns:

- **`Flag.Changed` pattern** (newer, preferred): `--host` ↔ `PKGPROXY_HOST` and
`--trust-proxy` ↔ `PKGPROXY_TRUST_PROXY`, both resolved in `serve`'s `PersistentPreRunE` via
`resolveListenHost` / `resolveTrustProxy` helpers that take a `flagChanged bool`.
- **value-equals-default heuristic** (older): `--config` ↔ `PKGPROXY_CONFIG`, which treats
`configPath == defaultConfigPath` as "not set". Its known edge case: `--config ./pkgproxy.yaml`
is indistinguishable from omitting the flag.

The `host-env-var` change (archived 2026-05-10) established the `Flag.Changed` approach as a
one-way ratchet for new code. This change follows it.

## Goals / Non-Goals

**Goals:**
- `podman run … -e PKGPROXY_CACHEDIR=/var/cache/pkgproxy ghcr.io/ganto/pkgproxy` uses that path
for the cache without appending `serve --cachedir …` to the entrypoint.
- Explicit `--cachedir` always wins, including `--cachedir cache` (the built-in default value).
- Local development (`make run`, `bin/pkgproxy serve`) keeps `cache` as the default — no change
to existing behavior when neither flag nor env var is set.

**Non-Goals:**
- Migrating `PKGPROXY_CONFIG` to the `Flag.Changed` pattern (separate change).
- Adding a `cachedir` key to the repository YAML config or the landing page snippets.
- Creating or validating the directory, or changing how `FileCache` treats the path.
- Baking a `PKGPROXY_CACHEDIR` default into the published image config.

## Decisions

### D1. Resolve in `serve`'s `PersistentPreRunE` via `cmd.Flag("cachedir").Changed`

`--cachedir` is an inherited persistent flag on `serve`, so `cmd.Flag("cachedir")` resolves it
from within the `serve` command's `PersistentPreRunE`. Add the helper and constant to
`cmd/root.go` (where `cacheDir`, `defaultDir`, and the `configPathEnvVar` constant already live),
and do the wiring in `cmd/serve.go` alongside the existing `resolveListenHost` /
`resolveTrustProxy` calls:

```go
// cmd/root.go
const cachedirEnvVar = "PKGPROXY_CACHEDIR"

func resolveCacheDir(flagChanged bool, flagValue, envValue string) string {
if flagChanged {
return flagValue
}
if envValue != "" {
return envValue
}
return defaultDir
}
```

```go
// cmd/serve.go — PersistentPreRunE, before initConfig()
cacheDir = resolveCacheDir(cmd.Flag("cachedir").Changed, cacheDir, os.Getenv(cachedirEnvVar))
```

`startServer` continues to read the package-level `cacheDir` unchanged.

**Alternatives considered:**
- _value-equals-default heuristic (mirror `PKGPROXY_CONFIG`)._ Rejected — carries forward the
edge case the project has already decided to stop propagating.
- _Add a `PersistentPreRunE` to the root command._ Rejected — the root command has none today,
`serve` already has one that does exactly this kind of resolution, and `serve` is the only
consumer of `cacheDir`.
- _viper for env binding._ Rejected — one more mapping doesn't justify a dependency.

### D2. Empty-string env var is treated as "unset"

`os.Getenv` returns `""` for both unset and explicitly-empty. Both fall through to the default.
An empty cache path is never useful, and this matches `resolveListenHost` / `resolveTrustProxy`.

### D3. Built-in default stays `cache`

`defaultDir` in `cmd/root.go` is unchanged. No Go-side conditional logic; container users set the
path at `podman run` time.

## Risks / Trade-offs

- **Two resolution patterns coexist in `cmd/`** (`Flag.Changed` for host/trust-proxy/cachedir,
value-equals-default for config) → documented deliberate ratchet; `PKGPROXY_CONFIG` migrates
later under its own change.
- **`Flag.Changed` on a package-global var can leak between command reruns in tests** → mitigate
by unit-testing `resolveCacheDir(changed, flagValue, envValue)` directly, with no Cobra
dependency, exactly as `TestResolveListenHost` does.
47 changes: 47 additions & 0 deletions openspec/changes/archive/2026-08-28-cachedir-env-var/proposal.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
## Why

The cache directory can only be set with the `--cachedir` flag. Every other operator-facing
setting (`--config`, `--host`, `--public-host`, `--trust-proxy`) has a `PKGPROXY_*` environment
variable so it can be configured in a container or an orchestrator without rewriting the command
line. `--cachedir` is the last gap: a containerized pkgproxy that stores its cache on a mounted
volume at a non-default path still needs a CLI argument appended to the default entrypoint.

## What Changes

- Add a `PKGPROXY_CACHEDIR` environment variable consulted by `serve` whenever the user has not
explicitly passed `--cachedir`. Resolution chain: `--cachedir` flag (when set by the user) →
`PKGPROXY_CACHEDIR` (when set and non-empty) → built-in default `cache`.
- Detect explicit user input with Cobra's `cmd.Flag("cachedir").Changed`, matching the
`PKGPROXY_HOST` / `PKGPROXY_TRUST_PROXY` precedent rather than the value-equals-default
heuristic still used by `PKGPROXY_CONFIG`. This keeps `--cachedir cache` distinguishable from
"no flag passed".
- An empty or unset `PKGPROXY_CACHEDIR` is treated as "no env-var input" and falls through to the
next step.
- Update the README.md flags table to fill in the env-var column for the `--cachedir` row.
- Add a `[Unreleased]` CHANGELOG entry.
- No new dependency (no viper); no change to the built-in default; no `.ko.yaml` change.

## Capabilities

### New Capabilities
- `cache-directory-config`: How `serve` resolves the local cache directory path from the
`--cachedir` flag, the `PKGPROXY_CACHEDIR` environment variable, and the built-in default.

### Modified Capabilities
_None._ This change is purely additive; no existing spec's requirements change.

## Impact

- `cmd/root.go` — Add a `cachedirEnvVar = "PKGPROXY_CACHEDIR"` constant near `configPathEnvVar`
and the `defaultDir` constant, and a `resolveCacheDir(flagChanged bool, flagValue, envValue string) string`
helper implementing flag → env → default precedence.
- `cmd/serve.go` — In `newServeCommand()`'s `PersistentPreRunE`, call
`resolveCacheDir(cmd.Flag("cachedir").Changed, cacheDir, os.Getenv(cachedirEnvVar))` and assign
the result back to `cacheDir` before `startServer` reads it. `--cachedir` is an inherited
persistent flag on the root command, so `cmd.Flag("cachedir")` resolves from `serve`.
- `cmd/root_test.go` (or `cmd/serve_test.go`) — Add `TestResolveCacheDir` mirroring the
table-driven style of `TestResolveListenHost`.
- `README.md` — Flags table: add `PKGPROXY_CACHEDIR` to the env-var column of the `--cachedir` row.
- `CHANGELOG.md` — One concise `[Unreleased]` entry.
- No changes to landing-page snippets (they describe client-side repo config, not server
invocation) or e2e tests (they invoke `serve` with explicit flags).
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
## ADDED Requirements

### Requirement: Cache directory is resolved from flag, then env var, then default
The `serve` subcommand SHALL resolve the local cache directory path using the following ordered
precedence, producing the value passed to the caching proxy as its cache base path:

1. The value of `--cachedir` when the user explicitly passed the flag on the command line
(detected via Cobra's `cmd.Flag("cachedir").Changed` returning `true`).
2. The value of the `PKGPROXY_CACHEDIR` environment variable when it is set to a non-empty string.
3. The built-in default `cache`.

An empty `PKGPROXY_CACHEDIR` (set but empty, or unset) SHALL be treated as "no env-var input" and
SHALL fall through to step 3. This change does not create, validate, or otherwise alter treatment
of the resolved path beyond selecting its value.

#### Scenario: Explicit `--cachedir` overrides everything
- **WHEN** the binary is started with `serve --cachedir /data/cache` and `PKGPROXY_CACHEDIR=/other` is set
- **THEN** the caching proxy SHALL use `/data/cache` as its cache base path

#### Scenario: Explicit `--cachedir cache` is honored
- **WHEN** the binary is started with `serve --cachedir cache` and `PKGPROXY_CACHEDIR=/data/cache` is set
- **THEN** the caching proxy SHALL use `cache` as its cache base path
- **AND** the env var SHALL NOT override the explicit flag value, even though it equals the built-in default

#### Scenario: `PKGPROXY_CACHEDIR` is used when the flag is absent
- **WHEN** the binary is started with `serve` (no `--cachedir`) and `PKGPROXY_CACHEDIR=/var/cache/pkgproxy` is set
- **THEN** the caching proxy SHALL use `/var/cache/pkgproxy` as its cache base path

#### Scenario: Empty `PKGPROXY_CACHEDIR` falls through to default
- **WHEN** the binary is started with `serve` (no `--cachedir`) and `PKGPROXY_CACHEDIR=` (set but empty)
- **THEN** the caching proxy SHALL use `cache` as its cache base path

#### Scenario: Neither flag nor env var produces the built-in default
- **WHEN** the binary is started with `serve` (no `--cachedir`) and `PKGPROXY_CACHEDIR` is unset
- **THEN** the caching proxy SHALL use `cache` as its cache base path
32 changes: 32 additions & 0 deletions openspec/changes/archive/2026-08-28-cachedir-env-var/tasks.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
## 1. Resolver helper and wiring

- [x] 1.1 Add `cachedirEnvVar = "PKGPROXY_CACHEDIR"` constant in `cmd/root.go` next to `configPathEnvVar` / `defaultDir`
- [x] 1.2 Add `resolveCacheDir(flagChanged bool, flagValue, envValue string) string` helper in `cmd/root.go` implementing flag → env → default (`defaultDir`) precedence
- [x] 1.3 In `cmd/serve.go` `newServeCommand()` `PersistentPreRunE`, call `cacheDir = resolveCacheDir(cmd.Flag("cachedir").Changed, cacheDir, os.Getenv(cachedirEnvVar))` before `initConfig()`

## 2. Unit tests

- [x] 2.1 Add `TestResolveCacheDir` (in `cmd/root_test.go`, or `cmd/serve_test.go` if it fits the existing resolver tests better) mirroring the table-driven style of `TestResolveListenHost`
- [x] 2.2 Cover: flag changed wins over env var; flag changed wins even when value equals default; env var used when flag unchanged; empty env var falls through to default; neither set returns `cache`
- [x] 2.3 Run `go test ./cmd/... -run TestResolveCacheDir` and confirm all subtests pass

## 3. Documentation

- [x] 3.1 In `README.md`, add `PKGPROXY_CACHEDIR` to the env-var column of the `--cachedir` row in the flags table
- [x] 3.2 Add a concise (80–100 char) entry under `## [Unreleased]` → `### Added` in `CHANGELOG.md` for the new env var

## 4. Validation

- [x] 4.1 Run `make ci-check` and confirm lint, govulncheck, and unit tests pass
- [x] 4.2 Run `pre-commit run --all-files` and resolve any findings
- [x] 4.3 Run `make e2e DISTRO=fedora` (at minimum) to confirm the e2e flow did not regress

## 5. Manual verification

Run from a clean shell so leftover env vars don't influence results.

- [x] 5.1 Build: `make build`
- [x] 5.2 Default (no flag, no env): `./bin/pkgproxy serve`, fetch a package, confirm it lands under `./cache/`
- [x] 5.3 Env var overrides default: `PKGPROXY_CACHEDIR=/tmp/pp-cache ./bin/pkgproxy serve`, fetch a package, confirm it lands under `/tmp/pp-cache/`
- [x] 5.4 Explicit flag wins over env: `PKGPROXY_CACHEDIR=/tmp/pp-cache ./bin/pkgproxy serve --cachedir ./cache`, confirm cache writes go to `./cache/` and `/tmp/pp-cache/` stays empty
- [x] 5.5 Empty env var falls through: `PKGPROXY_CACHEDIR= ./bin/pkgproxy serve`, confirm same behavior as 5.2
35 changes: 35 additions & 0 deletions openspec/specs/cache-directory-config/spec.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
## Requirements

### Requirement: Cache directory is resolved from flag, then env var, then default
The `serve` subcommand SHALL resolve the local cache directory path using the following ordered
precedence, producing the value passed to the caching proxy as its cache base path:

1. The value of `--cachedir` when the user explicitly passed the flag on the command line
(detected via Cobra's `cmd.Flag("cachedir").Changed` returning `true`).
2. The value of the `PKGPROXY_CACHEDIR` environment variable when it is set to a non-empty string.
3. The built-in default `cache`.

An empty `PKGPROXY_CACHEDIR` (set but empty, or unset) SHALL be treated as "no env-var input" and
SHALL fall through to step 3. This change does not create, validate, or otherwise alter treatment
of the resolved path beyond selecting its value.

#### Scenario: Explicit `--cachedir` overrides everything
- **WHEN** the binary is started with `serve --cachedir /data/cache` and `PKGPROXY_CACHEDIR=/other` is set
- **THEN** the caching proxy SHALL use `/data/cache` as its cache base path

#### Scenario: Explicit `--cachedir cache` is honored
- **WHEN** the binary is started with `serve --cachedir cache` and `PKGPROXY_CACHEDIR=/data/cache` is set
- **THEN** the caching proxy SHALL use `cache` as its cache base path
- **AND** the env var SHALL NOT override the explicit flag value, even though it equals the built-in default

#### Scenario: `PKGPROXY_CACHEDIR` is used when the flag is absent
- **WHEN** the binary is started with `serve` (no `--cachedir`) and `PKGPROXY_CACHEDIR=/var/cache/pkgproxy` is set
- **THEN** the caching proxy SHALL use `/var/cache/pkgproxy` as its cache base path

#### Scenario: Empty `PKGPROXY_CACHEDIR` falls through to default
- **WHEN** the binary is started with `serve` (no `--cachedir`) and `PKGPROXY_CACHEDIR=` (set but empty)
- **THEN** the caching proxy SHALL use `cache` as its cache base path

#### Scenario: Neither flag nor env var produces the built-in default
- **WHEN** the binary is started with `serve` (no `--cachedir`) and `PKGPROXY_CACHEDIR` is unset
- **THEN** the caching proxy SHALL use `cache` as its cache base path
Loading