-
Notifications
You must be signed in to change notification settings - Fork 284
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 #37347 - Add Packages wizard to All Hosts page #10962
base: master
Are you sure you want to change the base?
Conversation
8863469
to
4590f1e
Compare
7841521
to
9f40035
Compare
9f40035
to
9692fd3
Compare
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.
Seeing this following when hitting Next for all options except "Upgrade packages":
TypeError: isSelected is not a function
in RowSelectTd (created by Table)
in tr (created by TrBase)
in TrBase (created by Tr)
in Tr (created by Table)
in tbody (created by TbodyBase)
in TbodyBase (created by Tbody)
in Tbody (created by Table)
in table (created by TableComposableBase)
in TableComposableBase (created by TableComposable)
in TableComposable (created by Table)
in Table (created by TableIndexPage)
in section (created by PageSection)
in PageSection (created by TableIndexPage)
in div (created by TableIndexPage)
in TableIndexPage (created by HostReview)
in HostReview (created by BulkPackagesWizard)
in div (created by WizardBody)
in div (created by WizardBody)
in WizardBody (created by WizardToggle)
in div (created by WizardToggle)
in div (created by WizardToggle)
in WizardToggle (created by WizardInternal)
in WizardInternal (created by Wizard)
in div (created by Wizard)
in WizardContextProvider (created by Wizard)
in Wizard (created by BulkPackagesWizard)
in BulkPackagesWizard (created by BulkPackagesWizardModal)
in div (created by ModalBox)
in ModalBox (created by ModalContent)
in div (created by FocusTrap)
in FocusTrap (created by ForwardRef)
in ForwardRef (created by ModalContent)
in div (created by Backdrop)
in Backdrop (created by ModalContent)
in ModalContent (created by Modal)
in Modal (created by BulkPackagesWizardModal)
in BulkPackagesWizardModal
in Slot (created by ConnectFunction)
in ConnectFunction (created by HostsIndex)
in section (created by PageSection)
in PageSection (created by TableIndexPage)
in div (created by TableIndexPage)
in TableIndexPage (created by HostsIndex)
in HostsIndex (created by Context.Consumer)
in Route (created by AppSwitcher)
in Switch (created by ForemanSwitcher)
in ForemanSwitcher (created by AppSwitcher)
in AppSwitcher (created by ReactApp)
in ErrorBoundary (created by ReactApp)
in main (created by Page)
in div (created by Page)
in Page (created by Layout)
in div (created by FlexItem)
in FlexItem (created by Layout)
in div (created by Flex)
in Flex (created by Layout)
in Layout (created by ConnectedLayout)
in ConnectedLayout (created by ReactApp)
in Router (created by ConnectedRouter)
in ConnectedRouter (created by Context.Consumer)
in ConnectedRouterWithContext (created by ConnectFunction)
in ConnectFunction (created by ReactApp)
in ApolloProvider (created by ReactApp)
in div (created by ReactApp)
in ReactApp (created by I18nProviderWrapper(ReactApp))
in IntlProvider (created by I18nProviderWrapper(ReactApp))
in I18nProviderWrapper(ReactApp) (created by StoreProvider(I18nProviderWrapper(ReactApp)))
in Provider (created by StoreProvider(I18nProviderWrapper(ReactApp)))
in StoreProvider(I18nProviderWrapper(ReactApp)) (created by DataProvider(StoreProvider(I18nProviderWrapper(ReactApp))))
in DataProvider(StoreProvider(I18nProviderWrapper(ReactApp)))
in AwaitedMount
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.
The "Cancel" button seems to be non-functional
oh yeah I've been meaning to fix that 😄 I think it works on the review page at least |
Do you have the Foreman PR checked out? |
That must be it :) |
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 users can currently filter package versions by using the search bar, but I'm guessing they'd like to be able to see what versions are available. |
This bulk wizard only allows upgrading to the latest version. I can't think of a sane way to allow any granularity there, when you're dealing with multiple hosts that may have different versions installed and different versions available. |
That would be nice! But the version they're installing or upgrading to could vary from host to host, depending on that host's LCE/CV. |
Hit an error trying to install 4 RPMs on two hosts: (Really odd that it's bookmark related?)
|
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.
Confirmed that updating all & selected packages works fine. Also confirmed that the edit buttons work, along with run via rex and customized rex.
@ianballou Can you choose 'via customized' and see what you end up with for the host search query? Does it resolve to 0 or 2 hosts? |
|
The bookmark issue does not appear for custom REX. |
9692fd3
to
ffdcd25
Compare
@ianballou Bookmark error issue should be fixed now. |
Bookmark error is fixed, up next:
Confirmed vim-enhanced is available. Edit: vim-enhanced was installed on both hosts, which triggered the error. I was a little confused since it made it sound like the RPM wasn't among the synced content. I wonder if the error could be improved. |
ffdcd25
to
04a4fea
Compare
I just tested:
|
c0ca37e
to
1233f5f
Compare
hostname, search, hostSearch, versions, descriptionFormat, | ||
} = options; | ||
const searchQuery = hostSearch ?? `name ^ (${hostname})`; | ||
console.log(versions) |
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.
console.log
Things all seem to be working now! |
Looks like the bookmark clipping issue only happens when your screen is pretty narrow at least. But having it work with the side scroller would be cool if possible. |
I think we may need some help here on this part like with host details page :) |
Flipped the bookmark dropdown so it's entirely within the wizard window. This should fix the issue. (Will require updating both Foreman and Katello PR branches.) |
/packit build |
Account evgeni has no write access nor is author of PR! |
/packit build |
RPMs were built successfully. \o/ |
Still working just fine for me! The bookmark menu is now visible at all times. |
4f8ba03
to
672771d
Compare
672771d
to
76096b9
Compare
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.
APJ. Nice work 💯
76096b9
to
9567301
Compare
@@ -25,3 +25,4 @@ jobs: | |||
plugin: katello | |||
postgresql_container: ghcr.io/theforeman/postgresql-evr | |||
test_existing_database: false | |||
foreman_version: refs/pull/10128/head |
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.
Added this to run tests with the Foreman PR. Tests are green, except that our "React Tests" check seems to ignore the Foreman version specified. However, npm run test
passes locally (with the Foreman PR checked out):
Test Suites: 137 passed, 137 total
Tests: 584 passed, 584 total
Snapshots: 165 passed, 165 total
Requires theforeman/foreman#10128
What are the changes introduced in this pull request?
Add the Packages Wizard to the new All Hosts page, based on designs.
There are 4 steps in the wizard:
And there are 3 flows:
This also required adding features in Foreman React components, and adding new capabilities here:
:distinct
and:select
options toapi_controller.rb
so that you can get back a list distinct by package name. I had to write some SQL fragments myself since Active Record wasn't playing nice with.count
.package_names_for_job_template
if the package search turns up empty. This is to prevent the template from updating all packages when you only wanted to update one package.Considerations taken when implementing this change?
For the wizard itself, I used the "React next" implementation in PF4. I thought this was a much cleaner API because of the handy wizardContext it provides, and also thought it might be easier to migrate once we move to PF5.
For most of the wizard, I use a React Context,
BulkPackagesWizardContext
, to share values between all the wizard steps and components. But the<HostReview>
component should be reusable in other wizards, and using context would have made it nonreusable. So for<HostReview>
I made sure not to use any context values, and instead passed all values through props.Originally I had the
useBulkSelect
calls within each separate component (BulkPackagesTable
andHostReview
). And I was trying to share values by adding state to the top-level context. But I quickly realized that was a bad decision. Maybe surprisingly, things got a lot cleaner and more elegant once I moved bothuseBulkSelect
s to the top level.What are the testing steps for this pull request?