Packages step - second PR #1461
Conversation
lucasgarfield
force-pushed
the
packagesStep
branch
3 times, most recently
from
42dd55d
to
7f7a5ae
Compare
lucasgarfield
force-pushed
the
packagesStep
branch
from
7f7a5ae
to
3d24f2d
Compare
| const wildcardsUsed = filter.includes("*"); | ||
| const regex = / +|, +/g; | ||
| let filterValue = filter.replace(regex, ","); | ||
| const regexStrip = /(^,+)|(,+$)/g; | ||
| filterValue = filterValue.replace(regexStrip, ""); | ||
| filterValue = wildcardsUsed ? filterValue : `*${filterValue}*`.replace(/,/g, "*,*"); |
There was a problem hiding this comment.
I know you just copied this over from inputs.js but any chance you can explain what's happening here.
There was a problem hiding this comment.
I don't know what the purpose behind every line is because as you pointed out, it has just been copy/pasted it so that the the search results would be the same as before. But overall the purpose is to transform the search query into a form that will actually work with the Weldr API which expects you to use things like leading and trailing wildcards. So for instance, if you want to search for "vim", you actually need to query the Weldr API "*vim*" if you want it to behave as you would expect show packages such as neovim.
There was a problem hiding this comment.
wildcardsUsedis true if an*appears in the filterregexmatches a non-empty sequence of spaces, or a comma followed by a non-empty sequence of spacesfilterValuereplaces each match ofregexwith a comma - in other words all spaces are replaced by commas, without inserting repeated commasregexStripmatches any non-empty sequence of commas at the beginning or end of a stringfilterValueis update to replace leading or trailing strings of commas with the empty string- finally, as Lucas said, each match is replaced with wildcard versions, except if wildcards are already used.
I wonder if a future task could be to make weldr search better, if we push the search query into dnf-json we would get more powerful searching (making this logic redundant) and potentially much faster queries.
| const numPackages = | ||
| result.blueprints[0].blueprint.packages.length - result.blueprints[0].blueprint.modules.length; |
There was a problem hiding this comment.
totally fine but we really need to deal with this packages and modules situation. Mind creating an issue to track?
There was a problem hiding this comment.
See #1463.
However, with respect to this specific line, since we will now convert any modules to packages, modules.length should always be 0 and we can remove the subtrahend. And actually, this should have been the sum (+) and not the difference!
| const [packagesAvailablePage, setPackagesAvailablePage] = useState(1); | ||
| const [packagesAvailablePerPage, setPackagesAvailablePerPage] = useState(100); | ||
| const [additionalPackagesAvailableLoading, setAdditionalPackagesAvailableLoading] = useState(false); |
There was a problem hiding this comment.
At some point I would like to not paginate this so we can implement nicer sorting. Can you create an issue to track this?
There was a problem hiding this comment.
Done, see #1462.
| } | ||
| }, []); | ||
|
|
||
| const updateBlueprintComponents = (packageList) => { |
There was a problem hiding this comment.
Can we move the actual blueprint update to the final step? I would like the actual blueprint update to be on confirmation i.e. Create Image button or a Save Bluepoint button
There was a problem hiding this comment.
Great idea. The current behavior more closely resembles the existing cockpit-composer behavior, where adding or removing packages on the blueprintEdit page modifies the workspace. I would actually prefer it to behave more like Image Builder Front End, which would be the result of implementing your suggestion.
| // the fields isHidden and isSelected should not be included in the package list sent for image creation | ||
| const removePackagesDisplayFields = (packages) => | ||
| packages.map((pack) => ({ | ||
| name: pack.name, | ||
| summary: pack.summary, | ||
| })); |
There was a problem hiding this comment.
This is a helper specifically for the Packages component so can you include it inside of the Packages function.
There was a problem hiding this comment.
In fact, this is actually no longer necessary now that we set the packages form field to the list of package names and can just be removed.
| const packageInfo = await composer.getComponentInfo(packageNames); | ||
| const selectedPackages = packageInfo.map((pkg) => ({ name: pkg.name, summary: pkg.summary })); | ||
| setPackagesChosenSorted(selectedPackages); | ||
| change(input.name, removePackagesDisplayFields(selectedPackages)); |
There was a problem hiding this comment.
no need to change the field parameter like this. The packages with name and summary isn't used elsewhere. Instead, you could set the packages form field to the list of package names. That would enable you, on the review page, to load the packages names from the form state and not the blueprint.
There was a problem hiding this comment.
Good point! This does add some additional complexity to the review page, though, since the package list is fetched using an async API call. If a user clicks 'next' on the Packages step before the packages are loaded, there is a problem... when the API call completes the form state will be set correctly but the review step won't re-render to reflect this. My solution to this is to use a <FormSpy> to keep track of changes to the form state and re-render the review page when they occur.
| const formPackages = getState()?.values?.["selected-packages"]; | ||
|
|
||
| if (formPackages) { | ||
| setPackagesChosenSorted(formPackages); | ||
| } else { | ||
| fetchPackagesChosen(); |
There was a problem hiding this comment.
Can you explain this? It feels unnecessary to me.
There was a problem hiding this comment.
The useEffect() this belongs to executes every time the Packages step is rendered. However, we only want to fetch the blueprint packages via API calls the first time the Packages step is opened. If formPackages is undefined it is the first time the Packages step has been opened so we fetch the packages via an API call in the else clause.
There was a problem hiding this comment.
Actually, after changing the form state so that it is just the list of package names, we no longer keep track of the package summaries. That means that setPackagesChosenSorted(formPackages) no longer works, because it expects an array of objects of form { name, summary }.
I'm going to keep the form state as just the list of package names, and just call fetchPackagesChosen() every time the packages step is open.
This commit adds the packages step to the wizard. It is not functional at this point, the <Packages> component has been copy/pasted from image-builder-frontend and its API calls have been disabled.
lucasgarfield
force-pushed
the
packagesStep
branch
from
3d24f2d
to
769685e
Compare
The packages step is now fully functional, and the number of packages and dependencies are displayed on the review page.
lucasgarfield
force-pushed
the
packagesStep
branch
from
769685e
to
95a8878
Compare
There was a problem hiding this comment.
Overall, great work! This is good enough to merge but there are a few issues. Please add issues to track the FormSpy error. And, when adding packages, going to review page, then back to the packages step a user will see no packages added. Can you add an issue to track that as well?
simplescreenrecorder-2022-08-02_22.22.42.mp4 |
Thanks! And done, #1467
|
No description provided.