version placeholder/parsing, and os targets#1
version placeholder/parsing, and os targets#1davidgamero wants to merge 5 commits intoproject-dalec:mainfrom
Conversation
|
copying @cpuguy83 comment here for reference
|
Signed-off-by: David Gamero <david340804@gmail.com>
fa4a103 to
798a69e
Compare
Signed-off-by: David Gamero <david340804@gmail.com>
2ff79d8 to
d30a6cd
Compare
Signed-off-by: David Gamero <david340804@gmail.com>
d6067d3 to
e28a646
Compare
- Add test target to Makefile to run backend and UI tests - Configure vitest to run once and exit with --run flag - Add GitHub Actions workflow to run tests on push/PR - Refactor App.jsx to use clear variable names and if statements Signed-off-by: David Gamero <david340804@gmail.com>
e28a646 to
f8c7bf7
Compare
|
big diff line count, but mostly package lock and tests |
|
Gentle fyi @yolossn 🥂☕️ |
There was a problem hiding this comment.
Pull request overview
Adds version-constraint parsing and OS-target discovery to improve Dalec build UX, plus introduces Vitest-based test coverage and a CI workflow.
Changes:
- Add
parseVersionConstraints()utility (with Vitest tests) and update UI to emit version constraint arrays in the command preview + update version placeholder. - Replace UI OS list handling with
/api/os-backed OS targets plus a fallback list. - Add Vitest +
make testsupport for UI/backend and a GitHub Actions workflow to run tests on PRs/pushes.
Reviewed changes
Copilot reviewed 9 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| ui/src/utils/versionParser.js | New utility to split/normalize comma-separated version constraints. |
| ui/src/utils/versionParser.test.js | Adds Vitest coverage for version constraint parsing. |
| ui/src/App.jsx | Uses the parser for command preview dependency spec; fetches OS targets from backend; updates UI placeholder. |
| ui/package.json | Adds Vitest scripts and devDependencies. |
| ui/package-lock.json | Locks newly added Vitest-related dependencies. |
| backend/src/osProvider.js | Switches OS discovery to docker buildx targets JSON with fallback list. |
| backend/src/osProvider.test.js | Adds Vitest coverage for OS target parsing/fallback behavior. |
| backend/package.json | Adds Vitest test script and devDependency. |
| backend/package-lock.json | Locks newly added Vitest-related dependencies. |
| Makefile | Adds make test to run backend+ui test suites. |
| .github/workflows/test.yml | Adds CI workflow to run make test on PRs/pushes. |
Files not reviewed (2)
- backend/package-lock.json: Language not supported
- ui/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const [imageName, setImageName] = useState('my-minimal-image:0.1.0'); | ||
| const [osTargets, setOsTargets] = useState(OS_TARGETS); | ||
| const [osTarget, setOsTarget] = useState('azlinux3'); |
There was a problem hiding this comment.
osTarget is initialized to azlinux3 and never reconciled with the fetched osTargets list. If /api/os returns a list that doesn't include the current osTarget, MUI Select will have an out-of-range value and the generated build target/command can be invalid. Consider updating osTarget after fetching (or whenever osTargets changes) to ensure it always matches an available option (e.g., first element / fallback).
| test: ## Run tests for backend and UI | ||
| cd backend && npm test | ||
| cd ui && npm test |
There was a problem hiding this comment.
This test target should be declared phony (e.g., added to .PHONY) to avoid being skipped if a file named test exists in the repo.
| }); | ||
|
|
||
| it('preserves quotes inside constraint values', () => { | ||
| expect(parseVersionConstraints('>=1.0.0')).toEqual(['>=1.0.0']); |
There was a problem hiding this comment.
The test name "preserves quotes inside constraint values" doesn't match what the assertion checks (it doesn't include any quotes inside the constraint). Rename the test to reflect the actual behavior under test or update the input/assertion to cover quoted content.
| expect(parseVersionConstraints('>=1.0.0')).toEqual(['>=1.0.0']); | |
| expect(parseVersionConstraints('>=1."0".0, <2.\'0\'.0')).toEqual(['>=1."0".0', "<2.'0'.0"]); |
| export function fetchOsList() { | ||
| return new Promise((resolve) => { | ||
| exec('dalec os list', { timeout: 5000 }, (err, stdout) => { | ||
| const cmd = 'docker buildx build --call targets,format=json --build-arg BUILDKIT_SYNTAX=ghcr.io/project-dalec/dalec/frontend:latest -<<<"\\{\\}"'; |
There was a problem hiding this comment.
The cmd here-string currently passes \{\} (i.e., literal backslashes) to bash, which results in \{\} being sent to buildx rather than a valid {} JSON object. That likely makes the buildx call fail and forces fallback every time; pass a real {} (or use a safer stdin pipe) so dynamic target detection works as intended.
| const cmd = 'docker buildx build --call targets,format=json --build-arg BUILDKIT_SYNTAX=ghcr.io/project-dalec/dalec/frontend:latest -<<<"\\{\\}"'; | |
| const cmd = 'docker buildx build --call targets,format=json --build-arg BUILDKIT_SYNTAX=ghcr.io/project-dalec/dalec/frontend:latest -<<<"{}"'; |
There was a problem hiding this comment.
I think we can list the supported OS from the frontend itself using the ddClient. Please refer the usage here
dalec-docker-desktop-extension/ui/src/App.jsx
Line 284 in 182b6ed
There was a problem hiding this comment.
I tried to do this a couple ways with the ddClient, but there was no support for stdin which was necessary for the os listing command. Do you have an idea how to do this without stndin?
There was a problem hiding this comment.
You can provide a path to a file that's just {}
Equiv shell:
dir="$(mktemp -d)"
echo '{}' > "${dir}/Dockerfile"
docker buildx build --call targets,format=json --build-arg BUILDKIT_SYNTAX=ghcr.io/project-dalec/dalec/frontend:latest "${dir}"
There was a problem hiding this comment.
oh i see, okay i'd still have to create the file from the backend right? frontend won't have write access to make a temp file?
i'll test it out now
There was a problem hiding this comment.
frontend can't write files to disk even in tmp, so a viable pattern would be:
- backend writes a temp file with "{}" as contents
- pass that temp path to frontend via an api route?
- frontend calls ddclient with command including the temp file path

add placeholder to version demonstrating format >=1.0.0, <2.0.0
add validation and parsing for version range lists
add list of supported os targets