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

On-prem: read blueprints from disk (COMPOSER-2413) #2603

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

kingsleyzissou
Copy link
Collaborator

We can use the cockpit files api to read in blueprints from disk.

This PR adds commits to bring in the required dependencies, some convenience scripts and an initial commit using an RTK query function to read blueprints from file and pull the details into the store.

@lucasgarfield
Copy link
Collaborator

I can't wait to see this in action! I'm trying to get it running locally but can't get anything to appear in Cockpit. I noticed quite a few changes to the makefile... what should I do to get this to run?

@kingsleyzissou
Copy link
Collaborator Author

I can't wait to see this in action! I'm trying to get it running locally but can't get anything to appear in Cockpit. I noticed quite a few changes to the makefile... what should I do to get this to run?

Oh right, so we need to pull in some of the cockpit dependencies. Try make cockpit/devel, hopefully that does the trick. I am copying this all into a VM and then running that.

@kingsleyzissou kingsleyzissou force-pushed the cockpit_fileapi branch 5 times, most recently from 2e17a88 to dadbaec Compare November 28, 2024 13:13
@kingsleyzissou kingsleyzissou force-pushed the cockpit_fileapi branch 2 times, most recently from 6983959 to 85f4c58 Compare December 3, 2024 10:03
Copy link

codecov bot commented Dec 3, 2024

Codecov Report

Attention: Patch coverage is 8.21918% with 67 lines in your changes missing coverage. Please review.

Project coverage is 84.98%. Comparing base (9b69344) to head (5cca844).

Files with missing lines Patch % Lines
src/store/cockpitApi.ts 8.82% 31 Missing ⚠️
src/mocks/cockpit.ts 3.22% 30 Missing ⚠️
fec.config.js 0.00% 6 Missing ⚠️

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2603      +/-   ##
==========================================
- Coverage   85.23%   84.98%   -0.26%     
==========================================
  Files         175      176       +1     
  Lines       20253    20318      +65     
  Branches     1976     1977       +1     
==========================================
+ Hits        17263    17267       +4     
- Misses       2968     3029      +61     
  Partials       22       22              
Files with missing lines Coverage Δ
src/constants.ts 100.00% <100.00%> (ø)
fec.config.js 0.00% <0.00%> (ø)
src/mocks/cockpit.ts 3.22% <3.22%> (ø)
src/store/cockpitApi.ts 35.48% <8.82%> (-22.85%) ⬇️

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 9b69344...5cca844. Read the comment docs.

@kingsleyzissou kingsleyzissou force-pushed the cockpit_fileapi branch 2 times, most recently from c73c587 to 8dd626a Compare December 4, 2024 15:43
@kingsleyzissou kingsleyzissou force-pushed the cockpit_fileapi branch 7 times, most recently from 3640100 to 71de59a Compare December 6, 2024 14:13
@kingsleyzissou
Copy link
Collaborator Author

I had to change some typescript config which made this PR a lot bigger. I think if we want to go ahead with this, it would probably be better to split the typescript changes into a separate PR.

@lucasgarfield lucasgarfield changed the title On-prem: read blueprints from disk On-prem: read blueprints from disk (COMPOSER-2413) Dec 6, 2024
@lucasgarfield
Copy link
Collaborator

It's working!
image

@kingsleyzissou
Copy link
Collaborator Author

/retest

The on-prem version of this repo depends on the `Cockpit` project. Some
of the files from the `Cockpit` repo are needed to build and run this
project. These files are saved into the `pkg/lib` directory, so we need
ot add some plumbing for this into the project.

We also need to add aliases for `cockpit` and `cockpit/fsinfo` modules.
The downside is that we will need to create stub functions and types for
the cockpit packages so typescript doesn't complain  ¯\_(ツ)_/¯

Additionally, this commit adds some convenience scripts to the Makefile.
We need a toml package to parse the blueprint files, this appears to be
the most maintained package.
Add an initial commit to scan a preset directory for user blueprint
files. This makes use of the cockpit files api.
@@ -1,17 +1,52 @@
INSTALL_DIR := ~/.local/share/cockpit/image-builder-frontend

cockpit/all: devel-uninstall devel-install build
COCKPIT_REPO_URL = https://github.com/cockpit-project/cockpit.git
COCKPIT_REPO_COMMIT = b0e82161b4afcb9f0a6fddd8ff94380e983b2238
Copy link
Collaborator

Choose a reason for hiding this comment

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

We need a strategy for keeping this updated but we can figure that out later.

sed -i "s/.*FIRST_BOOT_SERVICE_DATA.*/export const FIRST_BOOT_SERVICE_DATA = '$(FISTBOOT_SERVICE)';/" $@

.PHONY: prep
prep: src/constants.ts
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this get used? Can we delete it?

.PHONY: prep
prep: src/constants.ts

.PHONY: install
Copy link
Collaborator

Choose a reason for hiding this comment

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

Delete.

install:
npm install

.PHONY: start
Copy link
Collaborator

Choose a reason for hiding this comment

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

Delete.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm a little worried here about co-mingling Cockpit "stuff" with Insights "stuff". Can we move things up one level to avoid this?

@@ -15,6 +15,11 @@
],
"exactOptionalPropertyTypes": true,
"strictNullChecks": true,
"allowSyntheticDefaultImports": true
"allowSyntheticDefaultImports": true,
"paths": {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Again, we have some co-mingling here. Maybe we can either eliminate this by moving everything up one level as previously suggested, or maybe we can have a base tsconfig.json and then extend it separately for Cockpit and Insights?

@@ -7,6 +7,7 @@
"npm": ">=7.0.0"
},
"dependencies": {
"@ltd/j-toml": "1.38.0",
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure we actually need this. Sorry for not seeing this before and pointing it out but the Insights service already supports importing on-prem blueprints and I think there's already a "toml" package we can use.

// TODO: maybe we should pull in the cockpit types here
// and keep them in the project. Not ideal because it may
// diverge, so we need to think about it.
export interface UserInfo {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I need to set up the tslint to enforce this but generally I'm trying to use types and not interfaces.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants