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 #35503 - Notify user only when new errata are added to repo #10285

Closed

Conversation

hao-yu
Copy link
Contributor

@hao-yu hao-yu commented Sep 22, 2022

No description provided.

@theforeman-bot
Copy link

Issues: #35503

@hao-yu
Copy link
Contributor Author

hao-yu commented Oct 12, 2022

@pmoravec @wbclark you guys think this idea is workable? If yes then I will un-draft it.

@pmoravec
Copy link
Member

I like that approach, it should definitely fix the repeated issues.

The only concern is how scalable it is - can the dynflow steps handle inputs/outputs with (possibly tens of) thousands of errata IDs?

Is there some limitation, @adamruzicka ?

@adamruzicka
Copy link
Member

I have no data to back this claim up, but it should be fine. Of course the more data you push through the slower it will be, but there is no explicit limitation. Unless we get to the ballpark of around tens of megabytes in size, it should be just fine.

@hao-yu
Copy link
Contributor Author

hao-yu commented Oct 13, 2022

I think it should be ok. The table ids should be much smaller than the packages data that we used to put in the package profile upload tasks and the content view publish task.

@wbclark
Copy link
Contributor

wbclark commented Oct 17, 2022

@hao-yu the approach looks nice. do you have bandwidth to finish it at the moment?

@hao-yu hao-yu force-pushed the fix_35503_notify_only_new_errata branch from 7b43fe6 to d6c295b Compare October 18, 2022 04:39
@hao-yu hao-yu marked this pull request as ready for review October 18, 2022 04:40
@hao-yu hao-yu force-pushed the fix_35503_notify_only_new_errata branch from d6c295b to 9edb0a1 Compare October 18, 2022 06:03
@pmoravec
Copy link
Member

Sounds great to me!

@hao-yu
Copy link
Contributor Author

hao-yu commented Oct 18, 2022

1 test failure seems unrelated to this patch

@chris1984
Copy link
Member

Tests are green now.

@wbclark
Copy link
Contributor

wbclark commented Oct 19, 2022

I'm running some live tests on this in my devel environment. Will do another review when that is complete.

(If anyone else is interested, I had to rebase this on latest master branch to get it to start)

@wbclark
Copy link
Contributor

wbclark commented Oct 27, 2022

My sincere apologies for the delay in finishing this review.

After thoroughly testing this in my development environment, I came up with a few recommendations to slightly improve the UX and mostly to make the behavior around updating the inputs more consistent (in the prior implementation, the run step never executes when contents_changed => false so the post-hoc input truncation never occurs).

To finish before the upcoming stabilization period, I've opened a new PR #10328 which amends the original commit (keeping a single commit and so that @hao-yu 's authorship is preserved) and also adds myself and @pmoravec as co-authors.

A complete write up of the changes is included, along with my testing steps.

I'm requesting to please close this PR and continue the discussion at #10328 ... thanks to all for the very helpful discussion.

@ianballou
Copy link
Member

Closing this since #10328 has merged.

@ianballou ianballou closed this Nov 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants