Conversation
7047995 to
f4408c0
Compare
f4408c0 to
4c67632
Compare
4c67632 to
43d5a27
Compare
Detect version field in RegDef files and validate against V2 schema when version is 2.0. Use logger for validation messages and move schema paths to constants.
- Add AuthConfig model for V2 authentication configuration - Add version and auth_config fields to Registry model - Add V2 routing in check_artifact_async with fallback to V1 - Add CloudAuthHelper for AWS/GCP cloud registry authentication - Add environment credential loading for V2 cloud registries - Add V2 cloud registry dependencies (boto3, google-auth) - Add comprehensive tests for V2 models and routing Preserves all V1 functionality including: - Nexus detection and URL conversion - Snapshot version resolution - URL-based artifact search
5ffc9b1 to
f556b46
Compare
0f21053 to
e2bc8bf
Compare
c4a4175 to
05467d1
Compare
- Simplified retry logic (2 retries, 5s fixed delay) - Reduced timeouts to reasonable values (60s search, 120s download) - Removed debug/diagnostic code - Removed unnecessary test files - Clean up code style
05467d1 to
186f166
Compare
Detect version field in RegDef files and validate against V2 schema when version is 2.0. Use logger for validation messages and move schema paths to constants.
…nto registry-v2-artifact-searcher
|
|
||
| auth_ref = getattr(registry.maven_config, 'auth_config', None) | ||
| if not auth_ref: | ||
| return None |
There was a problem hiding this comment.
we quietly return None without log
| try: | ||
| from qubership_pipelines_common_library.v1.maven_client import MavenArtifactSearcher | ||
| except ImportError: | ||
| MavenArtifactSearcher = None |
There was a problem hiding this comment.
Better to raise error here instead of using None, so it’s clear library is missing. if we return None here, exception will still be raised later at line 81, but failing early would make error clearer.
| helm_config: Optional[HelmConfig] = None | ||
| helm_app_config: Optional[HelmAppConfig] = None | ||
| version: Optional[str] = "1.0" | ||
| auth_config: Optional[dict[str, AuthConfig]] = None |
There was a problem hiding this comment.
I see you haven't made different models for reg def v2. do not mix 2 models with different schemas in one. use polymorphism
|
|
||
|
|
||
| # Timeout for MavenArtifactSearcher: (connect_timeout, read_timeout) | ||
| DEFAULT_SEARCHER_TIMEOUT = (30, 60) |
There was a problem hiding this comment.
specify all timeouts in one place with defaults. may be here artifact_searcher.utils.constants.
make it possible to set them from pipeline. ARTIFACT_SEARCHER_CONFIG: map
don't forget to put this new pipe param here and job parameters
| Returns AuthConfig or None. | ||
| """ | ||
| if artifact_type != "maven": | ||
| return None |
There was a problem hiding this comment.
for what this condition? artifact_type: has default maven value always. what sense of artifact_type if it is not used anywhere else ?
| max_retries = 2 | ||
| last_error = None | ||
| local_path = None | ||
| maven_url = None |
| searcher = await loop.run_in_executor(None, CloudAuthHelper.create_maven_searcher, app.registry, env_creds) | ||
|
|
||
| urls = await asyncio.wait_for( | ||
| loop.run_in_executor(None, partial(searcher.find_artifact_urls, artifact=maven_artifact)), |
There was a problem hiding this comment.
why do we use third-party synchronous library to search for an artifact If we already have asynchronous method in our library for v1, we just need to add auth for other registers and that all. We need to reconsider the using of this library.
| maven_url = None | ||
|
|
||
| # Retry on transient errors (401, timeout, expired) | ||
| for attempt in range(max_retries): |
There was a problem hiding this comment.
is it necessary to add mechanism of retries for both v1 and v2
| local_path = os.path.join(create_app_artifacts_local_path(app.name, version), os.path.basename(maven_url)) | ||
| os.makedirs(os.path.dirname(local_path), exist_ok=True) | ||
|
|
||
| download_success = await _v2_download_with_fallback( |
There was a problem hiding this comment.
no need to waste time downloading artifact when searching, in this method just need to find
There was a problem hiding this comment.
you can check check_artifact_by_full_url_async for example
| return await _check_artifact_v1_async(app, artifact_extension, version, cred=None, classifier="") | ||
|
|
||
| # Try to extract useful HTTP error details for debugging | ||
| if hasattr(e, 'response'): |
There was a problem hiding this comment.
use same method for v1 and v2 for error handling
|
|
||
| try: | ||
| await asyncio.wait_for( | ||
| loop.run_in_executor(None, lambda: searcher.download_artifact(url, str(local_path))), |
There was a problem hiding this comment.
we already have method for downloading artifact, why do we use it from third-party library again?
| ) | ||
|
|
||
| @staticmethod | ||
| def get_gcp_access_token(service_account_json: str) -> Optional[str]: |
There was a problem hiding this comment.
why do we use self-written authorization method if we have to use third-party library from qubership?
|
|
||
| try: | ||
| headers = {} | ||
| if auth_config.provider == "gcp": |
There was a problem hiding this comment.
why is there download implementation 3? we already have download either in our library or in third-paty one
| return None | ||
|
|
||
| @staticmethod | ||
| def get_gcp_credentials_from_registry(registry: Registry, env_creds: Optional[Dict[str, dict]]) -> Optional[str]: |
There was a problem hiding this comment.
why only for gcp? superfluous
| if 'SNAPSHOT' in version: | ||
| raise ValueError("SNAPSHOT is not supported version of Solution Descriptor artifacts") | ||
| # TODO: check if job would fail without plugins | ||
| def download_sd_by_appver(app_name: str, version: str, plugins: PluginEngine, env: Environment = None) -> dict[str, object]: |
There was a problem hiding this comment.
env: Environment = None not used
|
|
||
| # V1 fallback path or non-V2 registry - need credentials for HTTP download | ||
| cred = None | ||
| if app_def.registry.credentials_id and env_creds: |
There was a problem hiding this comment.
remove fallback here also
| if artifact_info: | ||
| template_url, _ = artifact_info | ||
| template_url, repo_info = artifact_info | ||
| # V2 optimization: artifact already downloaded by MavenArtifactSearcher |
There was a problem hiding this comment.
optimization not needed here
| template_url, _ = artifact_info | ||
| template_url, repo_info = artifact_info | ||
| # V2 optimization: artifact already downloaded by MavenArtifactSearcher | ||
| if isinstance(repo_info, tuple) and len(repo_info) == 2 and repo_info[0] == "v2_downloaded": |
There was a problem hiding this comment.
what is it v2_downloaded ?? repository name can't be v2_downloaded
| template_url, _ = artifact_info | ||
| template_url, repo_info = artifact_info | ||
| # V2 optimization: artifact already downloaded by MavenArtifactSearcher | ||
| if isinstance(repo_info, tuple) and len(repo_info) == 2 and repo_info[0] == "v2_downloaded": |
There was a problem hiding this comment.
isinstance(repo_info, tuple) and len(repo_info) == 2 needless
Summary
Add support for Registry V2 cloud registries in the Python artifact searcher and fix
build_envtests so CI passes.Issue
Internal need to:
scripts/build_envtests due to a [tests] package conflict.Breaking Change?
No existing flows are changed; V2 support is used only when configured.
Scope / Project
python/artifact-searcherscripts/build_envImplementation Notes
Added
CloudAuthHelperinartifact_searcherto:authConfigentries from RegDef V2.env_creds.MavenArtifactSearcherfor:Updated
build_envRegDef V2 handling to:2.0.authConfigreferences in V2 config sections.Fixed
build_envtests by:from tests.test_helpers import TestHelpers.Tests / Evidence
Existing pytest suites for:
python/envgene/envgenehelperscripts/build_envrun in the GitHub Actions [tests] job.
After the changes, the previous
ModuleNotFoundErrorinscripts/build_envtests is resolved and the suite runs to completion (subject to any functional test failures).Additional Notes
artifact_searcheralready requires.