Skip to content

Make Debian upstream repository configurable. - #229

Merged
andrew merged 2 commits into
git-pkgs:mainfrom
vpalatin:vpalatin/ubuntu
Aug 5, 2026
Merged

Make Debian upstream repository configurable.#229
andrew merged 2 commits into
git-pkgs:mainfrom
vpalatin:vpalatin/ubuntu

Conversation

@vpalatin

@vpalatin vpalatin commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Support overriding the Debian handler's upstream (e.g. Ubuntu archives)

Tested with PROXY_UPSTREAM_DEBIAN=http://archive.ubuntu.com/ubuntu to get Ubuntu Resolute packages.

@vpalatin

vpalatin commented Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

The lint failed with: lint: internal/config/config.go#L442 cyclomatic complexity 31 of func (*Config).LoadFromEnv is high (> 30) (gocyclo)
I can definitely add another commit here refactoring LoadFromEnv(), e.g. with a setEnvString helper function

func setEnvString(dst *string, key string) {
       if v := os.Getenv(key); v != "" {
               *dst = v
       }

is this the preferred way ? (in the same PR ? in the same commit or a different commit ?)

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

The env override refactor misplaces the LoadFromEnv GoDoc comment and the new upstream configurability path should be exercised by tests within the changed regions.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Pull request overview

This PR adds configuration support for overriding the Debian/APT handler’s upstream base URL (e.g., pointing at Ubuntu archives) and wires that through server startup and environment-variable loading.

Changes:

  • Extend NewDebianHandler to accept a configurable upstream URL (defaulting to the existing Debian upstream when unset).
  • Add Upstream.Debian to config defaults and PROXY_UPSTREAM_DEBIAN environment override support.
  • Update affected tests and call sites to use the new Debian handler constructor signature.
File summaries
File Description
internal/server/server.go Passes configured Debian upstream into the Debian handler at server start.
internal/handler/debian.go Adds an upstream URL parameter with defaulting + trailing-slash normalization.
internal/handler/download_test.go Updates Debian handler construction in download cache-miss test.
internal/handler/notfound_ecosystems_test.go Updates Debian handler construction in upstream-not-found test table.
internal/handler/debian_test.go Updates Debian handler construction in routes test.
internal/config/config.go Adds Upstream.Debian, default value, env override, and refactors env parsing helpers.
internal/config/config_test.go Verifies default and env override behavior for Upstream.Debian.
Review details
  • Files reviewed: 7/7 changed files
  • Comments generated: 3
  • Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

Comment thread internal/config/config.go
Comment thread internal/config/config.go Outdated
Comment thread internal/handler/download_test.go
@andrew

andrew commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

The setEnvString/setEnvBool helper approach is exactly right, and having it in this PR is fine — no need to split it out. Commit layout doesn't matter either way, happy to squash on merge.

One small thing the refactor introduced: the LoadFromEnv doc comment (the block listing the PROXY_* vars) now sits directly above setEnvString with no blank line between them, so godoc attaches it to the helper and LoadFromEnv itself ends up undocumented. Moving that block back down to sit on func (c *Config) LoadFromEnv() (with the two helpers defined above it) would sort it — that's what the two Copilot comments on config.go are getting at.

The third Copilot suggestion on download_test.go isn't needed; the config test already covers the override and strings.TrimSuffix doesn't need its own assertion. I've resolved that one.

Avoid triggering the linter about the cyclomatic complexity of the LoadFromEnv
function in later changes by refactoring it to use setEnvString/setEnvBool
helpers.

No functional change, just collapse ~29 repetitive if-blocks into single-line
calls to two small helpers.
Support overriding the Debian handler's upstream (e.g. Ubuntu archives)
via PROXY_UPSTREAM_DEBIAN or upstream.debian in the config file.

Tested with PROXY_UPSTREAM_DEBIAN=http://archive.ubuntu.com/ubuntu
to get Ubuntu Resolute packages.
@vpalatin

vpalatin commented Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

One small thing the refactor introduced: the LoadFromEnv doc comment (the block listing the PROXY_* vars) now sits directly above setEnvString with no blank line between them, so godoc attaches it to the helper and LoadFromEnv itself ends up undocumented. Moving that block back down to sit on func (c *Config) LoadFromEnv() (with the two helpers defined above it) would sort it — that's what the two Copilot comments on config.go are getting at.

The updated version should now fix the comment/doc

@andrew andrew left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good.

@andrew
andrew merged commit 800ffc6 into git-pkgs:main Aug 5, 2026
5 checks passed
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.

3 participants