Skip to content

headlamp-build: Add package.json with scripts from Makefile #2573

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

illume
Copy link
Collaborator

@illume illume commented Nov 15, 2024

This is to simplify using Headlamp so that people do not need to have gnu Make installed. Which isn't on Windows, and isn't on some Mac/unix systems.

Only has the start command t
Note: This package is not intended to be published, and so the name is not important at this stage.

  • Add backend and frontend related targets
  • npm start works in root assuming you have node and golang installed
  • updated contrib guide to remove Makefile mentions
  • test WSL ubuntu
  • test Mac
  • test Windows cmd / powershell

@dosubot dosubot bot added the size:M This PR changes 30-99 lines, ignoring generated files. label Nov 15, 2024
@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:M This PR changes 30-99 lines, ignoring generated files. labels Nov 15, 2024
@illume illume requested review from joaquimrocha and a team November 15, 2024 14:33
@sniok
Copy link
Collaborator

sniok commented Nov 15, 2024

nice! since this is just a beginning I'd like to suggest a convention for package script names which I've seen in other projects
where ":" character separates groups and "-" separates rest, like this:

frontend:run
frontend:install-ci
frontend:coverage
backend:run
backend:coverage-html
etc..

another suggestion is to use npm run --prefix frontend ... instead of cd frontend && npm run ...

also I'm not sure if it's just me but I'd like to avoid duplicated commands like run backend and backend run. I know this might be a convenient shortcut for people that already used to one way or the other but I think having just one variant might be cleaner

Copy link
Contributor

@joaquimrocha joaquimrocha left a comment

Choose a reason for hiding this comment

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

I thought it was simpler but I think we need to rethink what the scripts should be called rather than having a match to the make targets we had before.

@@ -18,33 +18,33 @@ redirects the requests to the defined proxies.
The backend (Headlamp's server) can be quickly built using:

```bash
make backend
npm run backend
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the run backend, even though matching what we had is more confusing, since this is not running backend but building it. Maybe npm run build:backend and npm run backend would actually try to run the backend. WDYT?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I will change npm run backend so it runs the backend. So it is the same as npm run backend-run. Same thing with npm run frontend.

I don't think npm run build:backend is a good idea to do or discuss now.

@dosubot dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. and removed size:L This PR changes 100-499 lines, ignoring generated files. labels Nov 15, 2024
This is to simplify using Headlamp so that people do not need to
have installed gnu Make. Which isn't on Windows, and isn't on
some Mac/unix systems.

Just starting with the start script, the rest can come later.

This package is not intended to be published, and so the name is
not important at this stage.

Signed-off-by: René Dudfield <[email protected]>
@illume illume marked this pull request as draft November 15, 2024 18:59
@k8s-triage-robot
Copy link

Unknown CLA label state. Rechecking for CLA labels.

Send feedback to sig-contributor-experience at kubernetes/community.

/check-cla
/easycla

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size:M This PR changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants