-
Notifications
You must be signed in to change notification settings - Fork 293
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 #37402 - add jquery-ui dependency #10982
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like an OK stopgap to me. Did you have a look at how many work it would be to actually drop it from Katello too?
yeah its used in some parts of the sync management (and maybe more, I didn't look after that). and I dont have capacity now to try to remove it from there since its spread through a few files |
Sounds good to me: the Katello team has plans to remove those pages anyway. It may be a waste of time trying to rework them just to drop jquery-ui while they're fully replaced soon after. |
Sync management is special, though, since it's actually ERB + vanilla JS + jQuery and doesn't have any Angular in it 😳 |
@jeremylenz Can we move jquery-ui to katello anyway? and makes plans to maybe refactor sync management later? |
@MariaAga If we're the only ones that use it, I'm fine with moving it to Katello for now. And yes we should also talk about redesigning sync status :) |
Wondering if this: https://bugzilla.redhat.com/show_bug.cgi?id=2274132 has anything to do with this change here and if it fixes this. |
@sjha4 The Foreman PR dropping it is not yet merged, so my guess is it's something else 🤷 |
is anything else needed for this pr? |
This will also require moving the dependency in https://github.com/theforeman/foreman-packaging from foreman to rubygem-katello package. Once that is done I guess it is only a matter of who is willing and allowed to push the buttons 😄 |
@m-bucher Thanks, added it: theforeman/foreman-packaging#11004 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks perfectly fine from a Katello standpoint. Waiting on https://github.com/theforeman/foreman-packaging/pull/11004/files to merge before merging this. Please ping us when the packaging PR and Foreman PRs are ready.
A rebase will be needed to get the tests green. |
@MariaAga looks like there's a conflict now with katello.gemspec. Feel free to ping me when the time to merge is right so we can do it quickly. |
we want to remove it in core since its no longer used theforeman/foreman#10142
so adding it here as only Katello seem to use it, and hopefully it will be removed here as well