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

app: fix headless mode not running #2893

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

Conversation

DMaxter
Copy link

@DMaxter DMaxter commented Feb 12, 2025

When I tried running both the version of headlamp in the Arch AUR (not official) and the AppImage in this repo I would get an error about a function not existing. This complete makes headlamp useless for my usecase in WSL2 (in one of the computers I tested).

This PR address these issues and performs the following changes:

1. Correctly uses promise when on headless mode

In https://github.com/headlamp-k8s/headlamp/blob/v0.28.1/app/electron/main.ts#L1413 , we are not awaiting the Promise returned on the previous line, so it caused the exception I first mentioned in the issue. It happened because serverProcess was null when reaching the attachServerEventHandlers function. Solved by chaining a then at the end of it

2. Create a temporary directory for the static assets

When using on Arch, after fixing the first exception, I got a permission denied with the AUR package. With the AppImage, it said the system was readonly. It tries to patch the baseURL in place, which is not possible for squashFS on AppImages.
The fix I provided creates a temporary folder in the system temp location, copies the assets there and uses that temporary directory as the new static assets directory, that is allowed to be written by the process

3. Add a timeout to starting the browser

I felt the need to add a timer to starting the frontend on the browser as it always opened before the backend was serving requests. This is just a simple timer before trying to open the browser with the link


Feel free to scrutinize my code as I am not a Go developer and also not very familiar with Electron

Fixes #2892

@dosubot dosubot bot added the size:M This PR changes 30-99 lines, ignoring generated files. label Feb 12, 2025
@illume
Copy link
Collaborator

illume commented Feb 12, 2025

Hello.

Thanks for reporting the issue, and for the PR!

The backend job is failing with a lint issue. You can run the linter locally for the backend with:
make backend-lint

I noticed this error in the App CI build jobs:
Error: cmd\headlamp.go:911:12: undefined: os.CopyFS

@DMaxter
Copy link
Author

DMaxter commented Feb 12, 2025

I ran that command locally and it worked. What I think might be happening is that I used a more recent version of go than what you are using. The function os.CopyFS was introduced in go 1.23 (https://pkg.go.dev/os#CopyFS) and the CI is using 1.22 (

)

How should we proceed? Should the go version be updated or does this CopyFS function need to be implemented other way?

@illume
Copy link
Collaborator

illume commented Feb 13, 2025

@DMaxter: ah ok, thanks.

Yeah, I've been meaning to update Headlamp to 1.23.x so I'll do that in a separate PR.

@DMaxter
Copy link
Author

DMaxter commented Feb 13, 2025

Alright. Let me know if there is something I can support with

@illume
Copy link
Collaborator

illume commented Feb 17, 2025

PR for new golang (still needs to be reviewed/merged)

@illume
Copy link
Collaborator

illume commented Feb 17, 2025

@DMaxter that golang update PR is merged now.

os.Exit(1)
}

defer os.RemoveAll(dir)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't understand this part.

Why is dir being removed when later it is being assigned to config.staticDir? (line 917)

Copy link
Author

Choose a reason for hiding this comment

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

From the very limited knowledge I have of Go, defer seems to be an instruction to make whatever is in front to be called after the function returns.

So, what happens here is:

  1. Create temporary folder
  2. Schedule the temporary folder to be removed
  3. Copy the contents to it
  4. Serve contents of temporary folder

When the function returns, it should call this function

Copy link
Author

@DMaxter DMaxter Feb 19, 2025

Choose a reason for hiding this comment

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

According to the error on CI seems like this code never runs, so better fix it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size:M This PR changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Headless mode not running
2 participants