Support content_view_environment_id for host and hostgroup#1977
Support content_view_environment_id for host and hostgroup#1977jeremylenz wants to merge 1 commit into
Conversation
|
|
Fixed an issue with the test playbook; it should now run on both Katello master and PR branch. |
|
requesting review from @evgeni when you have a chance :) |
| module.foreman_spec['content_view']['ensure'] = False | ||
| module.foreman_spec['lifecycle_environment']['ensure'] = False | ||
|
|
||
| if content_view_environments is not None and (converted_from_deprecated or (content_view is None and lifecycle_environment is None)): |
There was a problem hiding this comment.
The conversion logic in activation_key.py (lines ~312-361) and HostMixin._convert_cv_lce_to_cve are largely identical: API version detection, multi-CV guard, deprecation warning, lookup, fallback, CVE resolution, and ensure=False. The activation_key version differs only in how it extracts fallback IDs from the entity and what it does with the result (sets content_view_environments labels vs content_view_environment_id).
Not sure if it is worth to consider whether the common flow (detect version, guard multi-CV, warn, lookup, resolve CVE, suppress old params) could live in a shared helper, with the AK-specific parts (entity structure, output format) parameterized. It's up to you 😄
There was a problem hiding this comment.
Fair point — the two paths do share a lot of structure. I kept them separate for now since the differences are meaningful: AK converts to label strings and feeds into the existing content_view_environments handling, while HostMixin sets a numeric content_view_environment_id directly. The entity response structures also differ. But if we add more resources that need this, extracting a shared helper would make sense.
lfu
left a comment
There was a problem hiding this comment.
All tests passed for ansible-playbook test_deprecate_cv_lce.yml -e @test_cv_lce_vars.yml.
|
@evgeni hi again! Seems test failures are unrelated, right? |
| bugfixes: | ||
| - foreman_helper - use safe dictionary access for content_view_id/environment_id | ||
| in update workaround to prevent KeyError on newer Katello versions | ||
| (https://github.com/theforeman/foreman-ansible-modules/pull/1977) |
There was a problem hiding this comment.
IMHO this is not a bugfix for something that is user-visible today, so we don't need to mention it.
| - host, hostgroup - the ``content_view`` and ``lifecycle_environment`` parameters | ||
| are deprecated and will be removed in a future version | ||
| (https://github.com/theforeman/foreman-ansible-modules/pull/1977) |
There was a problem hiding this comment.
What is the replacement for the deprecated parametes? Host and Hostgroup modules do not accept content_view_environments like AK does.
There was a problem hiding this comment.
For hosts, the API accepts both content_view_environments (labels) and content_view_environment_ids. For host groups, it's a single content_view_environment or content_view_environment_id.
should I update those modules here?
There was a problem hiding this comment.
I know what the API accepts. But the modules need to expose that (well, the list thing, not the ids, that's something we do not expose usually) for users to be able to use it.
| "The deprecated 'content_view' and 'lifecycle_environment' " | ||
| "parameters cannot safely update it — they would overwrite " | ||
| "the existing multi-CV assignment. Use " | ||
| "'content_view_environments' or 'content_view_environment_ids' " |
There was a problem hiding this comment.
There is no content_view_environments parameter and content_view_environment_ids is correctly (!) marked as invisible so it can't be set by users either.
|
AK changes have been split out into #1982 per @evgeni's suggestion. This PR will be rebased once #1982 merges, and the outstanding review comments about host/hostgroup will be addressed here:
|
800adbe to
d0e5da9
Compare
d0e5da9 to
75cbe96
Compare
|
@evgeni I removed the deprecation warning from plugins/module_utils/foreman_helper.py since there's no replacement param yet. Do you want to add those replacement params here so we can do the deprecation? Or push that to a future PR? |
|
This makes it work for 4.21 users, right? |
75cbe96 to
56c9bb7
Compare
On newer Katello versions where the old content_view_id/ lifecycle_environment_id API params have been removed, internally convert to content_view_environment_id. Multi-CV entities are protected from accidental overwrite. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@evgeni Updated the host logic and updated the test playbook (expandable in the description). |
Summary
On newer Katello versions where the
content_view_id/lifecycle_environment_idAPI params have been removed, the host and hostgroup modules internally convertcontent_view/lifecycle_environmentto the new API params:content_view_environment_id(singular)content_view_environment_ids(array incontent_facet_attributes)Multi-CV entities are protected from accidental overwrite. Backward compatible — on older Katello versions the conversion is skipped and old params flow through natively.
Split from #1982 (activation_key changes, merged). Related Katello PR: Katello/katello#11753
Test plan
All 9 tests pass against both old and new Katello:
content_view_environmentscontent_view_environmentsidempotencyTest vars (test_cv_lce_vars.yml)
Test playbook (test_deprecate_cv_lce.yml)
Run with:
ansible-playbook test_deprecate_cv_lce.yml -e @test_cv_lce_vars.ymlPrerequisites:
allow_multiple_content_viewssetting enabled + a second CV promoted to a second LCE (for test 5)🤖 Generated with Claude Code