Skip to content

Conversation

sjha4
Copy link
Member

@sjha4 sjha4 commented Oct 7, 2024

What are the changes introduced in this pull request?

Move needs_publish attribute to base json rabl so index page has access to it along with details page.
The CV index page today doesn't have this attribute and hence always assumes it's false and always shows the No updates banner on publish wizard regardless of state of CV.

Considerations taken when implementing this change?

There are performance impacts of having the needs_publish attribute on CV index response cause this value is not indexed, rather calculated per CV. So it adds a performance hit when pagination size is big. We need to ensure this is an accepatable hit.

The other fix could be to add a CV details API call when publish wizard is opened from the index page and I am debating whether to go down that route depending on how people feel about it.

What are the testing steps for this pull request?

  1. Create a CV with some repos and publish a version.
  2. Now create a filter or any other change that requires a new version to be published.
  3. Go to CV index page and hit publish from the row actions and notice the No Updates warning doesn't show up on publish wizard.
  4. Go to CV > details > Publish new version and notice the No updates warning isn't there.

@ianballou
Copy link
Member

I made a script to create 2000 CVs. I'm going to see how performance loading the index page differs.

@ianballou ianballou self-requested a review October 22, 2024 21:24
@ianballou
Copy link
Member

I tested with a pagination of 500 and saw no significant difference in performance with and without this change.

Copy link
Member

@ianballou ianballou left a comment

Choose a reason for hiding this comment

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

Works well!

@sjha4 sjha4 merged commit 0851e1e into Katello:master Oct 23, 2024
27 checks passed
chris1984 pushed a commit that referenced this pull request Nov 4, 2024
chris1984 pushed a commit that referenced this pull request Nov 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants