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

Added image-builder-frontend repository as a Cockpit package #2424

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

mgold1234
Copy link
Collaborator

@mgold1234 mgold1234 commented Sep 11, 2024

Align Insights IB Service and IB On-Prem UI with Unified Blueprint and Image Functionality

  • Implemented identical UI components for both Insights IB service and IB on-prem.
  • Standardized blueprint functionalities, allowing creation, editing, and import/export operations.
  • Unified image-building capabilities based on blueprints.
  • Maximized code sharing and reuse by abstracting common functionalities.
  • Utilized RTK Query (RTKQ) for efficient communication with two distinct APIs: image-builder (service) and cloud-api (on-prem).
  • Packaged IB on-prem as an RPM, making it compatible with RHEL, CentOS, and Fedora distributions, and integrated it as a Cockpit plugin for streamlined deployment.

Copy link

codecov bot commented Sep 12, 2024

Codecov Report

Attention: Patch coverage is 31.25000% with 110 lines in your changes missing coverage. Please review.

Project coverage is 83.18%. Comparing base (1a7350e) to head (b3044aa).
Report is 2154 commits behind head on main.

Files with missing lines Patch % Lines
src/Components/ImagesTable/ImagesTable.tsx 21.51% 62 Missing ⚠️
src/AppCockpit.tsx 0.00% 29 Missing and 1 partial ⚠️
src/Router.tsx 0.00% 8 Missing ⚠️
src/store/cockpitApi.ts 52.94% 8 Missing ⚠️
src/Utilities/useGetEnvironment.ts 81.81% 2 Missing ⚠️

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2424      +/-   ##
==========================================
+ Coverage   75.71%   83.18%   +7.46%     
==========================================
  Files          33      156     +123     
  Lines         597    17374   +16777     
  Branches      144     1683    +1539     
==========================================
+ Hits          452    14452   +14000     
- Misses        139     2902    +2763     
- Partials        6       20      +14     
Files with missing lines Coverage Δ
src/Components/LandingPage/LandingPage.tsx 95.10% <100.00%> (ø)
...Components/sharedComponents/ImageBuilderHeader.tsx 100.00% <100.00%> (ø)
src/constants.ts 100.00% <100.00%> (ø)
src/store/emptyCockpitApi.ts 100.00% <100.00%> (ø)
src/store/index.ts 98.66% <100.00%> (ø)
src/Utilities/useGetEnvironment.ts 85.71% <81.81%> (ø)
src/Router.tsx 0.00% <0.00%> (ø)
src/store/cockpitApi.ts 52.94% <52.94%> (ø)
src/AppCockpit.tsx 0.00% <0.00%> (ø)
src/Components/ImagesTable/ImagesTable.tsx 81.66% <21.51%> (ø)

... and 2 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3c1f834...b3044aa. Read the comment docs.

Copy link
Member

@croissanne croissanne left a comment

Choose a reason for hiding this comment

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

Thank you! Just a general first comment, to make this a little easier to review, could you maybe split it out into several commits? Having a good description for each one? The current commit message doesn't describe all the changes made.

src/Components/ImagesTable/EmptyState.tsx Outdated Show resolved Hide resolved
src/Components/LandingPage/BlueprintEmpty.tsx Outdated Show resolved Hide resolved
fec.config.js Outdated Show resolved Hide resolved
cockpit/webpack.config.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
src/Router.tsx Outdated Show resolved Hide resolved
@mgold1234 mgold1234 force-pushed the first_cockpit branch 2 times, most recently from 08ce8c7 to fcdf7eb Compare September 12, 2024 16:25
@regexowl
Copy link
Collaborator

Is this a duplicate of #2412? Can you please close a PR that is not needed anymore?

@mgold1234
Copy link
Collaborator Author

@regexowl I moved #2412 to draft becuase I still need it, hope its ok

@regexowl
Copy link
Collaborator

@mgold1234 Yup, perfectly fine, thank you!

Copy link
Member

@croissanne croissanne left a comment

Choose a reason for hiding this comment

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

Cool that we can reuse the router :) Mostly style comments.

src/Router.tsx Outdated Show resolved Hide resolved
src/Router.tsx Outdated Show resolved Hide resolved
src/Router.tsx Outdated Show resolved Hide resolved
@mgold1234 mgold1234 force-pushed the first_cockpit branch 2 times, most recently from a3a6a8a to 9e93e2d Compare September 17, 2024 08:57
@mgold1234 mgold1234 force-pushed the first_cockpit branch 2 times, most recently from ce6617b to ce5cdaa Compare September 17, 2024 12:27
@mgold1234 mgold1234 force-pushed the first_cockpit branch 2 times, most recently from c2e1e23 to 4be467d Compare September 30, 2024 11:29
@mgold1234 mgold1234 force-pushed the first_cockpit branch 12 times, most recently from 9b32e44 to 0602a7f Compare October 7, 2024 08:02
changes that allow to build and run in both onPremise (Cockpit) and Saas environments.

Configured webpack with necessary loaders and plugins.
`AppCockpit` file and `cockpit:build`, `cockpit:install` scripts were added for Cockpit compatibility.

added alias to fec.config.js to seperate Saas modules
We need to integrate a new API service for managing blueprints and update the Redux store to handle blueprint-related data in a Cockpit environment.
This will allow us to fetch, store, and manage blueprints effectively within the application.
Created a new 'cockpitApi' service to handle the API calls related to blueprints in cockpit env.
Added a 'blueprintsReducer' to the Redux store to manage blueprint state.
Added a new 'emptyCockpitApi' file for the blueprint API setup, including type definitions and query configurations.
- define isOnPremise boolean variable to check if its onPrem env.
- define useGetFeatureFlag hook to return false in on-premise environments and dynamically call the appropriate hook
(useFlag or useFlagWithEphemDefault) based on the feature flag name.
Improved structure by avoiding hook calls inside conditionals to adhere to React rules
@mgold1234 mgold1234 force-pushed the first_cockpit branch 2 times, most recently from 3802aa7 to 68310a9 Compare October 8, 2024 08:52
} from './imageBuilderApi';

// Injects API endpoints into the service for querying blueprints
export const blueprintsReducer = api.injectEndpoints({
Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks good, but we need to teach it how to actually return a GetBlueprintsApiResponse object.

You'll want to use a queryFn, check the RTKQ docs here:
https://redux-toolkit.js.org/rtk-query/usage/customizing-queries#customizing-queries-with-queryfn

You might want to use the Cockpit Files API to read in the blueprints from a directory, docs here: https://cockpit-project.org/guide/latest/cockpit-file

We haven't 100% decided on the directory yet, but perhaps ~/.local/share/image-builder/blueprints.

Copy link
Member

@croissanne croissanne left a comment

Choose a reason for hiding this comment

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

This is looking quite good I think, some compiler errors however, but I like the usage of queryFn!

<div id="main"></div>
<script defer src="main.js"></script>
</body>
</html>
Copy link
Member

Choose a reason for hiding this comment

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

Can you indent this file properly?

offset: queryArg.offset,
},
}),
queryFn: async () => {
Copy link
Member

Choose a reason for hiding this comment

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

this is great, very neat!

if (isOnPremise) {
return false;
}
if (
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if a switch statement makes it easier to read here.

@@ -150,10 +153,12 @@ const CreateImageWizard = ({ isEdit }: CreateImageWizardProps) => {

// =========================TO REMOVE=======================
Copy link
Member

Choose a reason for hiding this comment

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

hmm is this comment still valid now? or should it move down?

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.

5 participants