Skip to content

fix: _parse_artifactory_base_url() ignores PROXY_REGISTRY_URL — lockfile reinstall fails (#614)#616

Open
chkp-roniz wants to merge 2 commits intomicrosoft:mainfrom
chkp-roniz:fix/proxy-registry-url-virtual-subdirectory
Open

fix: _parse_artifactory_base_url() ignores PROXY_REGISTRY_URL — lockfile reinstall fails (#614)#616
chkp-roniz wants to merge 2 commits intomicrosoft:mainfrom
chkp-roniz:fix/proxy-registry-url-virtual-subdirectory

Conversation

@chkp-roniz
Copy link
Copy Markdown
Contributor

Summary

Fixes #614.

  • Fix A: _parse_artifactory_base_url() now reads PROXY_REGISTRY_URL first, falling back to ARTIFACTORY_BASE_URL with a DeprecationWarning — matching RegistryConfig.from_env() precedence.
  • Fix B: Virtual subdirectory download path now checks dep_ref.is_artifactory() (Mode 1: explicit FQDN from lockfile) before falling through to the env var check (Mode 2). This matches the non-virtual path and the virtual file/collection paths.
  • Migrates all tests to canonical PROXY_REGISTRY_* env vars; adds backward-compat tests for the deprecated aliases.

Root cause

_parse_artifactory_base_url() only read the deprecated ARTIFACTORY_BASE_URL env var. Users setting only PROXY_REGISTRY_URL (the canonical var from #401) could not reinstall virtual subdirectory packages from lockfile because:

  1. The env var was never read, so no proxy was configured.
  2. The virtual subdirectory path lacked a Mode 1 (explicit FQDN) check, so lockfile-restored host/prefix metadata was ignored.

Test plan

  • 113 artifactory-specific tests pass (13 new)
  • 3683 total unit tests pass
  • E2E validated: PROXY_REGISTRY_URL + PROXY_REGISTRY_ONLY=1 fresh install and lockfile reinstall of virtual subdirectory package through real Artifactory proxy
  • Security review: no SSRF, no credential leaks, no internal data in diff
  • Backward compat: ARTIFACTORY_BASE_URL and ARTIFACTORY_ONLY still work with deprecation warnings

🤖 Generated with Claude Code

…ile reinstall fails for virtual subdirectory packages (microsoft#614)

_parse_artifactory_base_url() only read the deprecated ARTIFACTORY_BASE_URL
env var and never checked the canonical PROXY_REGISTRY_URL introduced in microsoft#401.
This caused lockfile-based reinstalls of virtual subdirectory packages to fail
when only PROXY_REGISTRY_URL was set.

Fix A: Read PROXY_REGISTRY_URL first, fall back to ARTIFACTORY_BASE_URL with
a DeprecationWarning — matching RegistryConfig.from_env() precedence.

Fix B: Add dep_ref.is_artifactory() check (Mode 1: explicit FQDN from
lockfile) to the virtual subdirectory path so lockfile-restored metadata is
used directly, before falling through to the env var check.

Also migrates all tests to canonical PROXY_REGISTRY_* env vars and adds
backward-compat tests for the deprecated aliases.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings April 7, 2026 20:12
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Fixes a lockfile reinstall failure for virtual subdirectory packages when users configure the registry proxy via the canonical PROXY_REGISTRY_* env vars, aligning Artifactory/proxy detection across code paths and adding regression tests.

Changes:

  • Update _parse_artifactory_base_url() to prefer PROXY_REGISTRY_URL and fall back to ARTIFACTORY_BASE_URL with a DeprecationWarning.
  • Ensure virtual subdirectory downloads honor lockfile-restored Artifactory metadata (dep_ref.is_artifactory()) before consulting env-driven proxy configuration.
  • Migrate/extend unit tests to cover canonical env vars plus backward-compat behavior for deprecated aliases.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
src/apm_cli/deps/github_downloader.py Aligns proxy base URL parsing with canonical env var precedence and fixes virtual subdirectory Mode 1 (lockfile FQDN) routing.
tests/unit/test_artifactory_support.py Updates existing tests to use canonical PROXY_REGISTRY_* vars and adds new regression tests for both fixes and deprecated-alias compatibility.

…ASCII, rename test class

- Delegate _parse_artifactory_base_url() to RegistryConfig.from_env() to
  eliminate duplicated env-var parsing logic and prevent future divergence.
- Replace non-ASCII box-drawing chars with plain ASCII in test comment.
- Rename TestArtifactoryOnlyMode → TestProxyRegistryOnlyMode.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@chkp-roniz
Copy link
Copy Markdown
Contributor Author

All three review comments addressed in 7ed9ca5:

  1. Non-ASCII box-drawing chars → replaced ── with -- (plain ASCII).
  2. Class renameTestArtifactoryOnlyModeTestProxyRegistryOnlyMode.
  3. DRY delegation_parse_artifactory_base_url() now delegates to RegistryConfig.from_env() directly, eliminating duplicated env-var parsing logic. Uses an uncached call (not self.registry_config) since this method must reflect the current env on each invocation.

All 3683 unit tests pass.

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.

bug: _parse_artifactory_base_url() ignores PROXY_REGISTRY_URL -- lockfile reinstall fails for virtual subdirectory packages

2 participants