fix: _parse_artifactory_base_url() ignores PROXY_REGISTRY_URL — lockfile reinstall fails (#614)#616
Open
chkp-roniz wants to merge 3 commits intomicrosoft:mainfrom
Conversation
Contributor
There was a problem hiding this comment.
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 preferPROXY_REGISTRY_URLand fall back toARTIFACTORY_BASE_URLwith aDeprecationWarning. - 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. |
Contributor
Author
|
All three review comments addressed in 7ed9ca5:
All 3683 unit tests pass. |
sergio-sisternes-epam
requested changes
Apr 10, 2026
Collaborator
sergio-sisternes-epam
left a comment
sergio-sisternes-epam
left a comment
There was a problem hiding this comment.
Nice fix, @chkp-roniz -- delegating to RegistryConfig.from_env() is the right call, eliminates the duplicated parsing that caused this drift in the first place. Tests are thorough.
One thing before we can approve: please add a CHANGELOG entry under ## [Unreleased] / ### Fixed per our contributing guide.
…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>
…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>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
chkp-roniz
force-pushed
the
fix/proxy-registry-url-virtual-subdirectory
branch
from
03e11a2 to
9be2d91
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #614.
_parse_artifactory_base_url()now readsPROXY_REGISTRY_URLfirst, falling back toARTIFACTORY_BASE_URLwith aDeprecationWarning— matchingRegistryConfig.from_env()precedence.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.PROXY_REGISTRY_*env vars; adds backward-compat tests for the deprecated aliases.Root cause
_parse_artifactory_base_url()only read the deprecatedARTIFACTORY_BASE_URLenv var. Users setting onlyPROXY_REGISTRY_URL(the canonical var from #401) could not reinstall virtual subdirectory packages from lockfile because:Test plan
PROXY_REGISTRY_URL+PROXY_REGISTRY_ONLY=1fresh install and lockfile reinstall of virtual subdirectory package through real Artifactory proxyARTIFACTORY_BASE_URLandARTIFACTORY_ONLYstill work with deprecation warnings🤖 Generated with Claude Code