Update tests for deprecated CV/LCE param removal#21644
Open
jeremylenz wants to merge 2 commits into
Open
Conversation
Contributor
Reviewer's GuideRefactors activation key, host, and hostgroup tests/fixtures to use combined content view environment identifiers ( Sequence diagram for resolving CV/LCE pair to ContentViewEnvironment IDsequenceDiagram
participant Fixture
participant ApiFactory
participant ContentViewEnvironmentAPI as ContentViewEnvironmentAPI
participant ActivationKeyAPI as ActivationKeyAPI
Fixture->>ApiFactory: get_cvenv_id(content_view, lifecycle_environment)
ApiFactory->>ContentViewEnvironmentAPI: list_content_view_environments(params)
ContentViewEnvironmentAPI-->>ApiFactory: result[results][0][id]
ApiFactory-->>Fixture: cvenv_id
Fixture->>ActivationKeyAPI: ActivationKey(content_view_environment_ids=[cvenv_id]).create()
ActivationKeyAPI-->>Fixture: ActivationKey instance
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
jeremylenz
added
the
No-CherryPick
Contributor
Author
|
Contributor
Author
|
Contributor
Author
|
Contributor
|
Collaborator
|
PRT Result |
Satellite-QE
added
the
PRT-Passed
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The new
get_cvenv_idhelper assumes thatlist_content_view_environmentsalways returns at least one result and blindly accessesresult['results'][0]['id']; consider adding a sanity check (and a clearer failure message) for the case where no matching content view environment is found to make test failures easier to diagnose.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `get_cvenv_id` helper assumes that `list_content_view_environments` always returns at least one result and blindly accesses `result['results'][0]['id']`; consider adding a sanity check (and a clearer failure message) for the case where no matching content view environment is found to make test failures easier to diagnose.
## Individual Comments
### Comment 1
<location path="tests/foreman/api/test_docker.py" line_range="516-520" />
<code_context>
cvv = cv.read().version[0].read()
cvv.promote(data={'environment_ids': lce.id, 'force': False})
+ cvenv_id = module_target_sat.api_factory.get_cvenv_id(cv, lce)
ak = module_target_sat.api.ActivationKey(
- content_view=cv, max_hosts=100, organization=org, environment=lce
+ content_view_environment_ids=[cvenv_id], max_hosts=100, organization=org
</code_context>
<issue_to_address>
**suggestion (testing):** Also assert environment is cleared when `content_view_environment_ids` is emptied
Since these removal tests now only check that `content_view` becomes `None` after `content_view_environment_ids = []`, please also assert that `ak.environment` is `None` after the update. This better validates the backward‑compat behavior on the nailgun side when the CVE mapping is removed.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
516
to
520
| ak = module_target_sat.api.ActivationKey( | ||
| content_view=content_view, environment=module_lce, organization=module_org | ||
| content_view_environment_ids=[cvenv_id], organization=module_org | ||
| ).create() | ||
| assert ak.content_view.id == content_view.id | ||
| assert ak.content_view.read().repository[0].id == repo.id |
Contributor
There was a problem hiding this comment.
suggestion (testing): Also assert environment is cleared when content_view_environment_ids is emptied
Since these removal tests now only check that content_view becomes None after content_view_environment_ids = [], please also assert that ak.environment is None after the update. This better validates the backward‑compat behavior on the nailgun side when the CVE mapping is removed.
LadislavVasina1
added
the
Stream
There was a problem hiding this comment.
Pull request overview
Updates Robottelo tests/fixtures/helpers to use Katello’s Content View Environment (CVE) identifiers (content_view_environment_ids / content_view_environment_id) instead of the deprecated separate CV/LCE parameters that are removed in Katello 4.21.
Changes:
- Added
APIFactory.get_cvenv_id()helper to resolve a CVE id from a Content View + Lifecycle Environment pair. - Updated Activation Key creation/update paths to set
content_view_environment_idsinstead ofcontent_view+environment. - Updated Host/HostGroup-related test data to use CVE ids (
content_view_environment_ids/content_view_environment_id) in content facet attributes and hostgroup creation.
Reviewed changes
Copilot reviewed 28 out of 28 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/new_upgrades/test_capsulecontent.py | Switch AK creation to CVE ids for upgrade coverage. |
| tests/foreman/ui/test_oscappolicy.py | Use CVE ids when creating a host with content facet attributes. |
| tests/foreman/ui/test_hostcollection.py | Use CVE ids for hosts used in host collection tests. |
| tests/foreman/ui/test_host.py | Replace host content facet CV/LCE ids with CVE ids in UI flows. |
| tests/foreman/ui/test_errata.py | Update host content facet updates to use CVE ids. |
| tests/foreman/ui/test_activationkey.py | Create AKs using content_view_environment_ids. |
| tests/foreman/endtoend/test_api_endtoend.py | Use CVE ids for content hosts in end-to-end scenario. |
| tests/foreman/destructive/test_infoblox.py | Provisioning host content facet uses CVE ids. |
| tests/foreman/api/test_subscription.py | Update AK fixtures and host content facet updates to CVE ids. |
| tests/foreman/api/test_provisioningtemplate.py | Provisioning template tests create hosts with CVE ids. |
| tests/foreman/api/test_provisioning.py | PXE provisioning test sets CVE ids for host content facet. |
| tests/foreman/api/test_provisioning_puppet.py | Puppet provisioning test sets CVE ids for host content facet. |
| tests/foreman/api/test_hostgroup.py | Hostgroup/host content facet updated; hostgroup uses content_view_environment_id. |
| tests/foreman/api/test_host.py | Host creation/update flows use content_view_environment_ids. |
| tests/foreman/api/test_docker.py | AK create/update uses content_view_environment_ids and clearing uses empty list. |
| tests/foreman/api/test_contentview.py | Host subscription and AK rolling-CV paths use CVE ids; updates clear via ids. |
| tests/foreman/api/test_activationkey.py | AK tests create/search using content_view_environment_ids. |
| robottelo/host_helpers/api_factory.py | Add get_cvenv_id() and update registration helper to set CVE ids on AK. |
| pytest_fixtures/core/contenthosts.py | Content host registration fixture creates AK via CVE ids. |
| pytest_fixtures/component/rh_cloud.py | RH cloud activation key fixtures use CVE ids. |
| pytest_fixtures/component/repository.py | Repository fixture creates AK via CVE ids. |
| pytest_fixtures/component/provision_vmware.py | VMware hostgroup fixture uses content_view_environment_id. |
| pytest_fixtures/component/provision_pxe.py | PXE provisioning fixtures use CVE ids for AK/hostgroup. |
| pytest_fixtures/component/provision_ocpv.py | OCPV hostgroup fixture uses content_view_environment_id. |
| pytest_fixtures/component/provision_capsule_pxe.py | Capsule PXE fixtures use CVE ids for hostgroup/AK. |
| pytest_fixtures/component/leapp_client.py | Leapp AK fixture uses CVE ids. |
| pytest_fixtures/component/contentview.py | CV/LCE activation key fixture uses CVE ids. |
| pytest_fixtures/component/activationkey.py | Activation key fixtures consistently use content_view_environment_ids. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| result = self._satellite.api.ContentViewEnvironment().list_content_view_environments( | ||
| params={'content_view_id': cv_id, 'lifecycle_environment_id': lce_id} | ||
| ) | ||
| return result['results'][0]['id'] |
Comment on lines
665
to
675
| hosts = [] | ||
| for _ in range(2): | ||
| hosts.append( | ||
| module_target_sat.api.Host( | ||
| organization=module_org_with_parameter, | ||
| location=smart_proxy_location, | ||
| content_facet_attributes={ | ||
| 'content_view_id': cv.id, | ||
| 'lifecycle_environment_id': lce.id, | ||
| 'content_view_environment_ids': [ | ||
| module_target_sat.api_factory.get_cvenv_id(cv, lce) | ||
| ], | ||
| }, |
Update all API tests, fixtures, and helpers to use content_view_environment_ids (activation keys, hosts) and content_view_environment_id (hostgroups) instead of the removed separate content_view_id and lifecycle_environment_id params. Add get_cvenv_id() helper to api_factory for looking up ContentViewEnvironment IDs from CV+LCE pairs. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
jeremylenz
force-pushed
the
remove-deprecated-cv-lce-params
branch
from
6e86d38 to
b2ffb02
Compare
Satellite-QE
removed
the
PRT-Passed
Contributor
Author
|
pre-commit.ci autofix |
for more information, see https://pre-commit.ci
LadislavVasina1
added
the
PRT-Passed
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.
Problem Statement
Katello 4.21 removes the deprecated separate
content_view_idandlifecycle_environment_idAPI parameters from activation keys, hosts, and hostgroups. These were replaced bycontent_view_environment_ids/content_view_environment_idin 6.19.Solution
get_cvenv_id()helper toapi_factoryfor looking up ContentViewEnvironment IDs from CV+LCE pairscontent_view_environment_ids=[cvenv_id]instead ofcontent_view=cv, environment=lcecontent_facet_attributesto usecontent_view_environment_idsinstead of separatecontent_view_id/lifecycle_environment_idcontent_view_environment_id=cvenv_idinstead of separatecontent_view/lifecycle_environmentRead-side assertions (
ak.content_view.id,ak.environment.read(), etc.) are preserved via nailgun backward-compat properties.Related Issues
🤖 Generated with Claude Code
Summary by Sourcery
Migrate tests and helpers to use content view environment IDs instead of separate content view and lifecycle environment parameters.
New Features:
Enhancements:
Tests: