activation key: add support for multi cv

[martin-schlossarek]

This pull request adds support for multiple content views in activation keys. The tests have been updated accordingly, and the fixtures have been regenerated.

[evgeni]

When I tried implementing CV Environments for hosts in #1866, I used an "entity list", not a string list here (as CV Environments are entities that you can search for etc).

However, now that you did it differently, I wonder what's the most "correct" way is.

• The user needs to pass in the environment labels in both cases, so the user interface doesn't change.
• Using a "real" entity has the benefit of having a more direct error message if the CVE label can't be found, at the cost of an additional API request (see below)

Error with your code:

fatal: [localhost]: FAILED! => {"changed": false, "msg": "Failed to ensure entity state: ForemanApiException: Error while performing create on activation_keys: 422 Client Error: Unprocessable Content for url: https://centos9-stream-katello-nightly.juuni.example.com/katello/api/activation_keys - {'displayMessage': 'No content view environments found with names: RHEL9/blafasel', 'errors': ['No content view environments found with names: RHEL9/blafasel']}"}

Error with my code:

fatal: [localhost]: FAILED! => {"changed": false, "msg": "Found no results while searching for content_view_environments with label=\"RHEL9/blafasel\""}

I think paying an API request just to validate the label is too much, so I'd go with your solution, but wanted to highlight the possibility nevertheless

[martin-schlossarek]

I should also say that I based my work on how the API currently does things.

Right now, the API lets you pass in a list of content-view-environment labels or a list of environment IDs (entities). I implemented the first one. The second one would push things more toward an entity-list approach, like you mentioned.

TBH, I just went with the more pragmatic option because I want to use this feature soon and needed something that works. I played around with an entity-list version for a bit, but since I didn’t have the time or the deeper knowledge of the code, I decided to drop it for now.

[evgeni]

Thanks a lot Martin! This looks very good!

I left a few inline comments, but they are pretty much cosmetic.

Could you please add a small YAML snippet with a changelog entry in changelogs/fragments/? Something like changelogs/fragments/activation_key-cv_environments.yml:

minor_changes:
  - activation_key - add ``content_view_environments`` parameter to support multi CV (https://github.com/theforeman/foreman-ansible-modules/pull/1935)

[martin-schlossarek]

Thanks for the hints! I updated the code and adjusted the tests.

Regarding labels vs entities: I also would prefer an entity based implementation, but I really struggled with that flatten logic for content view environments. Maybe it would be an option to rename the parameter to content_view_environment_labels? This leaves open the option for an entity based implementation in the future...?

[evgeni]

Given there is no user difference whether we use entities or not: ACK!

I'll wait with the merge until @jeremylenz or @parthaa had a chance to look at this

[jeremylenz]

Maybe it would be an option to rename the parameter to content_view_environment_labels? This leaves open the option for an entity based implementation in the future...?

I wouldn't want to rename it, since that would affect the --content-view-environments parameter in Hammer. But aliasing them seems fine, so I opened https://projects.theforeman.org/issues/38937 in Katello.

I didn't test this but LGTM from a Katello / multiCV perspective! Thanks again @martin-schlossarek ❤️