Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes #38048 - Add rolling content views #11240

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

quba42
Copy link
Contributor

@quba42 quba42 commented Nov 28, 2024

@nadjaheitmann
Copy link
Contributor

Here is a question to @jeremylenz I guess.
I saw that you added the content_view_environment controller, recently. I am asking myself if this part of the code needs change: https://github.com/Katello/katello/blob/master/app/views/katello/api/v2/content_view_environments/show.json.rabl#L12 (and maybe some parts that use this code which I don't know :) )

@jeremylenz
Copy link
Member

I am asking myself if this part of the code needs change: https://github.com/Katello/katello/blob/master/app/views/katello/api/v2/content_view_environments/show.json.rabl#L12 (and maybe some parts that use this code which I don't know :) )

A content view environment should be created for each rolling content view. The label would be Library/<rolling_cv_label>. (The first part would always be Library/ since rolling CVs cannot be promoted to other LCEs.)

I would expect that rabl to work "for free" - so hammer content-view-environment list would display CVEs for rolling content views with labels like I described above. Is that what you are seeing? (I haven't tested this yet)

@quba42
Copy link
Contributor Author

quba42 commented Dec 3, 2024

A content view environment should be created for each rolling content view. The label would be Library/<rolling_cv_label>. (The first part would always be Library/ since rolling CVs cannot be promoted to other LCEs.)

I went and double checked. We are currently doing this by calling async_task(::Actions::Katello::ContentView::AddToEnvironment, @content_view.create_new_version, @content_view.organization.library) whenever we create a rolling CV. The resultant Library/<rolling_cv_label> environment is created both in the Katello and candlepin DB as expected.

I would expect that rabl to work "for free" - so hammer content-view-environment list would display CVEs for rolling content views with labels like I described above. Is that what you are seeing? (I haven't tested this yet)

I am getting Error: No such sub-command 'content-view-environment'. most likely because I am on a test instance based on Katello 4.13.1 right now. My Forklift (which is on the state of this PR) does not have Hammer installed. I will need to try to rectify this.

Manisha15 and others added 2 commits December 10, 2024 14:01
Co-authored-by: Markus Bucher <[email protected]>
Co-authored-by: Nadja Heitmann <[email protected]>
Co-authored-by: Quirin Pamp <[email protected]>
Co-authored-by: Bernhard Suttner <[email protected]>
* Fix sanitize content_view repository_ids param

Refs OR-5274
@quba42
Copy link
Contributor Author

quba42 commented Dec 10, 2024

We have decided to split out the top commit on this PR into a separate PR and issue:

The reasons for doing so are as follows:

  • This is arguably a bug that pre-dates the changes introduced with this PR.
  • The fix affects the way the API processes data, and we wanted this to receive the proper attention in review. As part of all the changes on this PR it is too easily overlooked!

@quba42
Copy link
Contributor Author

quba42 commented Dec 10, 2024

We believe this PR is now "ready for review".

To summarize:

@quba42 quba42 marked this pull request as ready for review December 10, 2024 14:06
Copy link
Member

@jeremylenz jeremylenz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking great!

Tested and working pretty much as expected.
Tested with

  • host assigned to multiple content view environments
  • attempting to disable RH repo included in rolling CV
  • attempting to remove repo from product included in rolling CV
  • hammer content-view-environment list

Some outstanding UI things:

On the Red Hat Repositories page when you go to disable a repository, there's a tooltip that says "cannot be disabled because it is a part of a published content view." Should that be updated to say something like "..part of a content view" or "..part of a published or rolling content view"?

I like the icon you chose to denote rolling type (vs. CV/CCV). That should also be updated

  • in new All Hosts page (ForemanColumnExtensions)
  • on host details ContentViewDetails card

Some more wording (cc @maximiliankolb) and other suggestions below

id="component"
title={__('Content view')}
onClick={() => { setComponent(true); setComposite(false); }}
title={__('Regular content view')}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it will be too confusing for downstream customers to introduce a new term "Regular." We've had extensive discussion in the past about composite vs. non-composite, and what to call components of a composite content view. In the end we settled on just "Content view."

Suggested change
title={__('Regular content view')}
title={__('Content view')}

isSelected={component}
>
{__('Single content view consisting of e.g. repositories')}
{__('Contains a set of versioned and optionally filtered repositories')}
Copy link
Member

@jeremylenz jeremylenz Dec 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(From the user perspective*) it's not the repositories that are versioned, but the content view itself. Perhaps

Contains repositories. Each version is a snapshot which must be published and may optionally be filtered.

* I know Pulp has repository versions but who knows what those are ;)

isSelected={composite}
>
{__('Consisting of multiple content views')}
{__('Contains a set of regular content views')}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
{__('Contains a set of regular content views')}
{__('Contains a set of content views')}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants