Fixes #38048 - Add rolling content views #11240

quba42 · 2024-11-28T12:35:55Z

Issue: https://projects.theforeman.org/issues/38048

Community discussion: https://community.theforeman.org/t/transparent-content-view-for-limiting-repository-access-in-katello/39389

Hammer changes to go with this feature: Katello/hammer-cli-katello#974

nadjaheitmann · 2024-12-02T14:12:26Z

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 · 2024-12-02T14:57:52Z

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 · 2024-12-03T08:51:24Z

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.

quba42 · 2024-12-10T13:54:44Z

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 · 2024-12-10T14:06:17Z

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

To summarize:

As it stands, this PR requires the following to be merged first: Fixes #38076 - Sanitize content_view repository_ids param #11253, afterwards it can be rebased on top as a single commit. (Right now the change is also part of the branch for this PR).
There is also an accompanying Hammer PR: Refs #38048 - Add rolling content views hammer-cli-katello#974
There is also an accompanying docs PR: Add docs for rolling content views theforeman/foreman-documentation#3518
The full list of authors and co-authors for this PR is: @Manisha15, @nadjaheitmann, @m-bucher, @sbernhard, and @quba42 with additional review by @maximiliankolb and possibly others. 😉
The current state has experienced extensive pair programming, ATIX internal review, and ATIX internal manual testing.

jeremylenz

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

webpack/scenes/ContentViews/Create/CreateContentViewForm.js

quba42 · 2025-01-10T10:27:21Z

@jeremylenz The following should now be the only remaining ToDo from the things you mentioned:

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

in new All Hosts page (ForemanColumnExtensions)

on host details ContentViewDetails card

I will continue with this on Monday.

Once that is done, I will re-base and squish everything together.

It would also be great if we could get #11253 reviewed and merged soonish, since this PR depends on it (and maintaining multiple branches is annoying).

quba42 · 2025-01-16T14:48:47Z

The latest push fixes several UI things including the things mentioned by @jeremylenz. In particular:

The new UI (all hosts and host details) now uses the rolling CV icon where appropriate.
The "Default Organization View" in the new UI also uses the rolling CV icon (instead of the icon for a standard CV).
In both old and new UI rolling CVs no longer link to or list their dummy "Version 1.0". Instead they just display the environment "Library" in those places (similar to the "Default Organization View").

The following screen shot is illustrative of the new state:

quba42 · 2025-01-16T15:37:36Z

Looks like I now broke some UI tests. I don't expect to get around to fixing them before Monday.

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]>

quba42 · 2025-01-20T13:15:49Z

Tests are green, and everything is squished and rebased.
=> Ready for the next (and potentially final) round of review.

My understanding is that the plan is to postpone merging until after Katello 4.16 has branched.
Which I believe is planned for mid February.

So far, and now that #11253 has been merged, this branch has been relatively easy to maintain (no VCR recordings), so I am fine with finalizing review now, and then just occasionally rebasing the branch on top of master as we approach the next Katello branching.

sjha4 · 2025-01-27T19:41:53Z

webpack/scenes/ContentViews/Create/CreateContentViewForm.js

              isSelected={component}
            >
-              {__('Single content view consisting of e.g. repositories')}
+              {__(contentViewDescriptions.CV)}


Not sure this translation will work..Better to translate strings in contentViewDescriptions directly.

It won't work.

Marking a review on this so we don't forget.

ianballou

Initial testing is generally looking good, some questions:

ianballou · 2025-01-27T22:17:38Z

webpack/scenes/ContentViews/Create/CreateContentViewForm.js

              isSelected={component}
            >
-              {__('Single content view consisting of e.g. repositories')}
+              {__(contentViewDescriptions.CV)}


Marking a review on this so we don't forget.

...ack/components/extensions/HostDetails/Cards/ContentViewDetailsCard/ContentViewDetailsCard.js

ianballou · 2025-01-27T22:49:45Z

webpack/scenes/ContentViews/Details/ContentViewDetails.js

+
+  const dropDownItems = [];
+  if (!rolling) {
+    dropDownItems.push(<DropdownItem key="copy" ouiaId="cv-copy" onClick={() => { setCopying(true); }} > {__('Copy')}</DropdownItem>);


Why is copying not supported for rolling CVs?

To be honest, I guess we considered it as a follow-up feature (if needed).

ianballou · 2025-01-27T22:54:02Z

app/lib/actions/katello/repository/sync.rb

@@ -56,6 +57,7 @@ def plan(repo, options = {})
              end
              plan_self(:id => repo.id, :sync_result => output, :skip_metadata_check => skip_metadata_check, :validate_contents => validate_contents,
                        :contents_changed => output[:contents_changed])
+              update_rolling_content_views(repo)


Was having the rolling CV update be asynchronous considered? I suppose it's likely that users won't have many rolling content views, so the sync/upload shouldn't likely be blocked too long, but I'm curious if it was a consideration.

I guess we did, but since there is currently no progress bar for "Add repository", it would not be visible for the user that this runs in background.

As you said, it was sufficiently fast (at least in the tests I did), so blocking has not been an issue, yet.

If we want to do it asynchronously, we definitely will need an indication in the UI that it is running in the background.

pr-processor bot added Not yet reviewed Waiting on contributor labels Nov 28, 2024

sbernhard requested a review from jeremylenz November 29, 2024 09:56

nadjaheitmann mentioned this pull request Dec 2, 2024

Refs #38048 - Add rolling content views Katello/hammer-cli-katello#974

Open

quba42 force-pushed the rolling_cv branch from 5999d04 to 8d68c30 Compare December 10, 2024 13:03

quba42 mentioned this pull request Dec 10, 2024

Fixes #38076 - Sanitize content_view repository_ids param #11253

Merged

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

maximiliankolb mentioned this pull request Dec 11, 2024

Add docs for rolling content views theforeman/foreman-documentation#3518

Draft

2 tasks

jeremylenz reviewed Dec 12, 2024

View reviewed changes

webpack/scenes/ContentViews/Create/CreateContentViewForm.js Outdated Show resolved Hide resolved

webpack/scenes/ContentViews/Create/CreateContentViewForm.js Outdated Show resolved Hide resolved

webpack/scenes/ContentViews/Create/CreateContentViewForm.js Outdated Show resolved Hide resolved

pr-processor bot removed the Not yet reviewed label Dec 12, 2024

ianballou self-requested a review January 8, 2025 14:06

pr-processor bot added Needs re-review and removed Needs re-review labels Jan 9, 2025

quba42 force-pushed the rolling_cv branch from eadaca8 to 72f723a Compare January 9, 2025 20:58

pr-processor bot added the Needs re-review label Jan 9, 2025

quba42 force-pushed the rolling_cv branch from 72f723a to 2cfe5b5 Compare January 10, 2025 10:16

quba42 force-pushed the rolling_cv branch from df224e9 to 7158f43 Compare January 16, 2025 14:38

Fixes #38048 - Add rolling content views

b515dc2

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]>

quba42 force-pushed the rolling_cv branch from 7158f43 to b515dc2 Compare January 20, 2025 11:28

pr-processor bot removed the Waiting on contributor label Jan 20, 2025

sjha4 reviewed Jan 27, 2025

View reviewed changes

pr-processor bot removed the Needs re-review label Jan 27, 2025

ianballou requested changes Jan 27, 2025

View reviewed changes

pr-processor bot added the Waiting on contributor label Jan 27, 2025

fixup! Fixes #38048 - Add rolling content views

3940f88

pr-processor bot added Needs re-review and removed Needs re-review labels Feb 5, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #38048 - Add rolling content views #11240

Fixes #38048 - Add rolling content views #11240

quba42 commented Nov 28, 2024 •

edited

Loading

nadjaheitmann commented Dec 2, 2024

jeremylenz commented Dec 2, 2024

quba42 commented Dec 3, 2024

quba42 commented Dec 10, 2024

quba42 commented Dec 10, 2024 •

edited

Loading

jeremylenz left a comment •

edited

Loading

quba42 commented Jan 10, 2025 •

edited

Loading

quba42 commented Jan 16, 2025

quba42 commented Jan 16, 2025

quba42 commented Jan 20, 2025

sjha4 Jan 27, 2025

ianballou Jan 27, 2025

ianballou Jan 27, 2025

ianballou left a comment

ianballou Jan 27, 2025

ianballou Jan 27, 2025

m-bucher Feb 5, 2025

ianballou Jan 27, 2025

m-bucher Feb 5, 2025

Fixes #38048 - Add rolling content views #11240

Are you sure you want to change the base?

Fixes #38048 - Add rolling content views #11240

Conversation

quba42 commented Nov 28, 2024 • edited Loading

nadjaheitmann commented Dec 2, 2024

jeremylenz commented Dec 2, 2024

quba42 commented Dec 3, 2024

quba42 commented Dec 10, 2024

quba42 commented Dec 10, 2024 • edited Loading

jeremylenz left a comment • edited Loading

Choose a reason for hiding this comment

quba42 commented Jan 10, 2025 • edited Loading

quba42 commented Jan 16, 2025

quba42 commented Jan 16, 2025

quba42 commented Jan 20, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ianballou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

quba42 commented Nov 28, 2024 •

edited

Loading

quba42 commented Dec 10, 2024 •

edited

Loading

jeremylenz left a comment •

edited

Loading

quba42 commented Jan 10, 2025 •

edited

Loading