refactor: manage Artifactory credentials via context manager - #5811
refactor: manage Artifactory credentials via context manager#5811rnetser wants to merge 14 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
📝 WalkthroughWalkthroughThe change adds managed Artifactory credentials, updates DataVolume and VM helpers to use them, consolidates test fixtures, and adds lifecycle and failure-path tests. ChangesArtifactory credential lifecycle
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
sig-iuo
sig-chaos
sig-infra
sig-scale
sig-storage
sig-virt
labels
|
Report bugs in Issues Welcome! 🎉This pull request will be automatically processed with the following features: 🔄 Automatic Actions
📋 Available CommandsPR Status Management
Review & Approval
Testing & Validation
Container Operations
Cherry-pick Operations
Branch Management
Label Management
✅ Merge RequirementsThis PR will be automatically approved when the following conditions are met:
📊 Review ProcessApprovers and ReviewersApprovers:
Reviewers:
Available Labels
AI Features
Security Checks
💡 Tips
📌 Additional InformationCustom Commands:
For more information, please refer to the project documentation or contact the maintainers. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5811 +/- ##
==========================================
+ Coverage 98.89% 98.91% +0.02%
==========================================
Files 42 42
Lines 2631 2682 +51
==========================================
+ Hits 2602 2653 +51
Misses 29 29
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
openshift-virtualization-qe-bot
added
the
tox:verify-tc-requirement-polarion:running
|
@EdDev check this out |
|
/retest all Auto-triggered: Files in this PR were modified by merged PR #5948. Overlapping filestests/conftest.py |
|
/retest all Auto-triggered: Files in this PR were modified by merged PR #5898. Overlapping filestests/conftest.py |
|
/retest all Auto-triggered: Files in this PR were modified by merged PR #5813. Overlapping filestests/storage/conftest.py |
Replace get/cleanup pairs with artifactory_credentials so Secret/ConfigMap lifecycle is tied to with/ExitStack and cannot leak on partial failure. Assisted-by: Cursor Grok 4.5 <noreply@cursor.com> Signed-off-by: rnetser <rnetser@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Avoid implicit get_client() for Secret/ConfigMap create/delete in chaos, storage, tekton, and instance-types fixtures. Assisted-by: Cursor Grok 4.5 <noreply@cursor.com> Signed-off-by: rnetser <rnetser@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Make DynamicClient mandatory so Secret/ConfigMap create and delete never rely on implicit get_client(). Assisted-by: Cursor Grok 4.5 <noreply@cursor.com> Signed-off-by: rnetser <rnetser@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Drop optional DynamicClient defaults so low-level Artifactory helpers never fall back to implicit get_client(). Assisted-by: Cursor Grok 4.5 <noreply@cursor.com> Signed-off-by: rnetser <rnetser@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Use from-import instead of utilities.artifactory.artifactory_credentials per project import conventions. Assisted-by: Cursor Grok 4.5 <noreply@cursor.com> Signed-off-by: rnetser <rnetser@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Pass the namespace's own client instead of a separate unprivileged_client fixture into artifactory_credentials for the Windows DV template. Assisted-by: Cursor Grok 4.5 <noreply@cursor.com> Signed-off-by: rnetser <rnetser@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Use the owning namespace (or resource) client instead of a separate admin_client/unprivileged_client fixture arg where a Namespace object is available. Assisted-by: Cursor Grok 4.5 <noreply@cursor.com> Signed-off-by: rnetser <rnetser@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Track whether get_artifactory_* deployed Secret/ConfigMap so nested artifactory_credentials contexts do not delete pre-existing shared resources still needed by longer-lived owners. Assisted-by: Cursor Grok 4.5 <noreply@cursor.com> Signed-off-by: rnetser <rnetser@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Ensure artifactory_credentials does not delete a borrowed Secret if ConfigMap creation raises after Secret lookup. Assisted-by: Cursor Grok 4.5 <noreply@cursor.com> Signed-off-by: rnetser <rnetser@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
…failure When dv.create() or dv.wait_for_dv_success() raises after some DVs are already created, the post-yield cleanup never runs. Use ExitStack to register per-DV cleanup via stack.callback immediately after each create, so partial failures still tear down everything created so far. Signed-off-by: rnetser <rnetser@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
- Update create_dv docstring to document selective Secret/ConfigMap creation and cleanup scope accurately - Extract inline artifactory_credentials in windows_data_volume_template into a dedicated windows_instance_type_artifactory_credentials fixture for consistency with the pattern used across all other conftest files Signed-off-by: rnetser <rnetser@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
ExitStack.callback() has a positional-only 'callback' parameter. Using callback=dv.clean_up raises TypeError on Python 3.14+. Switch to positional form and add 'callback' to fcn_exclude_functions in .flake8 to suppress the false-positive FCN001 lint error (same pattern as enter_context). Signed-off-by: rnetser <rnetser@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/storage/conftest.py`:
- Around line 373-375: Update artifactory_credentials_scope_module to satisfy
pytest scope ordering by using a module-scoped namespace fixture, or reduce the
credential fixture to function scope; preserve the existing credential yield and
avoid depending on a narrower-scoped namespace.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8e466e90-3340-4888-b01f-fe822b4cd2be
📒 Files selected for processing (4)
tests/conftest.pytests/storage/conftest.pyutilities/storage.pyutilities/unittests/test_storage.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
RedHatQE/openshift-virtualization-tests-design-docs(manual)
- Remove unused @patch decorators from error-path unit tests - Prefix unused mock args with underscore (ARG002/ARG005) - Check Secret/ConfigMap existence before reading credentials - Apply ExitStack pattern to scale DVs for correct teardown order Signed-off-by: rnetser <rnetser@redhat.com>
|
@coderabbitai[bot] The following review comments were reviewed and a decision was made:
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== Artifactory module outline =="
ast-grep outline utilities/artifactory.py --items all --type function,class --match 'get_artifactory_secret|get_artifactory_config_map|artifactory_credentials|ArtifactoryCredentials' --view expanded
echo
echo "== Artifactory credential implementation and helper context =="
rg -n -C 35 \
'^(def get_artifactory_secret\b|def get_artifactory_config_map\b|def artifactory_credentials\b|class ArtifactoryCredentials\b)|os\.environ|read_text|\.exists\(|\.deploy\(|\.clean_up\(' \
utilities/artifactory.py
echo
echo "== Scale fixture outline =="
ast-grep outline tests/scale/test_scale_benchmark.py --items all --type function --match 'golden_images_scale_dvs' --view expanded
echo
echo "== Scale fixture implementation =="
rg -n -C 55 \
'^def golden_images_scale_dvs\b|ExitStack|enter_context|stack\.callback|artifactory_credentials|\.clean_up\(' \
tests/scale/test_scale_benchmark.py
echo
echo "== All Artifactory credential context usages =="
rg -n -C 4 \
'\b(artifactory_credentials\s*\(|enter_context\s*\(\s*artifactory_credentials)' \
--glob '*.py' \
. || trueLength of output: 37735 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== Context-manager cleanup block =="
sed -n '260,320p' utilities/artifactory.py
echo
echo "== Relevant Artifactory context-manager tests =="
rg -n -C 24 \
'def test_.*(cleanup|existing|overlap|nested|owner|borrower|shared)|artifactory_credentials\(' \
utilities/unittests/test_artifactory.py
echo
echo "== Direct context-manager callers and their cleanup scope =="
for file in utilities/oadp.py tests/utils.py tests/fixtures/images/validation_os_images.py \
tests/data_protection/oadp/conftest.py tests/observability/metrics/utils.py \
tests/virt/cluster/longevity_tests/utils.py; do
echo "--- $file ---"
rg -n -C 18 \
'\bwith artifactory_credentials\b|\byield\b|\.clean_up\(|delete_resources\b|return\b' \
"$file" || true
doneLength of output: 117050
HIGH — creation-only inputs: Addressed. HIGH — cleanup order: Addressed. In HIGH — overlapping contexts: The skipped finding remains valid. I cannot withdraw it.
with artifactory_credentials(namespace, client): # Context A creates resources.
with artifactory_credentials(namespace, client): # Context B borrows resources.
# If Context A exits before Context B, A deletes the resources.
# Context B can still require the resources.The cleanup block in Fixture scope ordering can prevent the issue for specific fixtures with enforced scopes. It cannot protect direct calls to A coordinated lifetime mechanism, such as per-namespace reference tracking, is required if overlapping contexts are supported. Otherwise, the API must explicitly forbid overlapping contexts and enforce that restriction. You are interacting with an AI system. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/scale/test_scale_benchmark.py`:
- Around line 258-262: Update the setup flow around artifactory_credentials and
delete_resources so keep_resources governs both credential resources and
DataVolumes: register both cleanup actions, then detach or otherwise disable the
registered cleanup callbacks after successful setup when resources must be
retained. Preserve normal cleanup when keep_resources is false and ensure the
credentials remain available alongside retained DataVolumes.
- Around line 261-262: Update the DataVolume setup flow around DataVolume.deploy
and dvs_list so each DataVolume is registered for cleanup before deployment,
ensuring deploy failures are also cleaned up. Keep setup cleanup active while
deployment is in progress, and disable it only after setup succeeds when
keep_resources is true; rely on delete_resources handling uncreated DataVolumes.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 48f6dd79-4257-4648-b378-6ff69f2e4651
📒 Files selected for processing (4)
tests/scale/test_scale_benchmark.pyutilities/artifactory.pyutilities/unittests/test_artifactory.pyutilities/unittests/test_storage.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
RedHatQE/openshift-virtualization-tests-design-docs(manual)
- Append DataVolume to cleanup list before deploy() to prevent leaks - Use stack.pop_all() when keep_resources=True to retain credentials Signed-off-by: rnetser <rnetser@redhat.com>
|
@coderabbitai review |
|
|
@coderabbitai review |
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
/retest all Auto-triggered: Files in this PR were modified by merged PR #5672. Overlapping filestests/conftest.py |
7 participants
What this PR does / why we need it:
Replace Artifactory Secret/ConfigMap
get_*+cleanup_*pairs with anartifactory_credentialscontext manager (ArtifactoryCredentialsdataclass) so lifecycle is tied towith/ExitStackand resources cannot leak on partial create failure.client: DynamicClientis mandatory onartifactory_credentials,get_artifactory_secret, andget_artifactory_config_map(no implicitget_client()). Call sites pass an explicit client.create_dvcomposes selective create via ExitStack (create_secret/create_config_map). Unit tests cover the new API andcreate_dvwiring.Assisted-by: Cursor Grok 4.5 noreply@cursor.com
Signed-off-by: rnetser rnetser@redhat.com
Which issue(s) this PR fixes:
Special notes for reviewer:
.flake8addsenter_contexttofcn_exclude_functionssoExitStack.enter_context(...)is allowed (project FCN exception list; not a# noqa).utilities/storage.pyis coverage-omitted; added focusedTestCreateDvArtifactoryunit tests for the new ExitStack/artifactory paths.jira-ticket:
NONE
Summary by CodeRabbit
Refactor
Tests