-
Notifications
You must be signed in to change notification settings - Fork 0
Allow creation of executable binary combined frontend/backend #38
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds support for building a standalone executable (via PyInstaller) alongside the existing Docker workflow, restructures the API under /api
, and updates test and CI configurations to handle both Docker and compiled binary deployments.
- Introduce
deploy_as_executable
flag incopier.yml
and adjust CI to build or pull artifacts accordingly - Update frontend/backend routing to use
/api/graphql
, adjust test harness, and add deterministic Faker seeding - Enhance GitHub Actions to include compiled‐binary build/test steps and optimize Docker layers
Reviewed Changes
Copilot reviewed 41 out of 41 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
template/frontend/.dockerignore | Exclude test files and .dockerignore from build context |
template/frontend/codegen.ts | Point GraphQL schema to /api/graphql |
template/frontend/vitest.config.ts | Register setup files for Faker and application launcher |
template/frontend/tests/setup/faker.ts | Seed Faker deterministically before each test |
template/frontend/tests/setup/constants.ts.jinja | Export deployed ports and application name |
template/frontend/tests/setup/app.ts | New E2E harness launching either Docker or compiled executable backend |
template/frontend/tests/e2e/index.spec.ts | Switch to shared getPage /url helpers and remove redundant setup |
template/frontend/tests/e2e/constants.ts.jinja | Remove outdated port constant |
template/frontend/tests/compiled/index.spec.ts | Add a compiled‐frontend smoke test |
template/frontend/package.json.jinja | Refine test scripts, add --run flag and include playwright |
template/frontend/nuxt.config.ts.jinja | Add runtime public config for graphql_api_url |
template/deployment/docker-compose.yaml.jinja | Update GRAPHQL_API_URL to use /api/graphql |
template/README.md.jinja | Update uvicorn entrypoint path |
template/.pre-commit-config.yaml.jinja | Exclude favicons from large file checks |
template/.github/workflows/ci.yaml.jinja | Extend matrix and add compiled build/test jobs |
template/.github/actions/install_deps_uv/manual-setup-deps.ps1 | Adjust backend path resolution in CI action |
template/.devcontainer/devcontainer.json.jinja | Enable shared Python modules when using executable deploy |
template/.coveragerc | Remove entrypoint.py from omit list |
copier.yml | Add deploy_as_executable and conditional parameters |
.github/workflows/ci.yaml | Include an extra copier data file in CI tests |
throw new Error(`Failed to stop the application: ${res.statusText}`); | ||
} | ||
} | ||
if (isDockerE2E) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the Docker E2E branch you never close the Playwright browser. Consider moving await browser.close()
outside the conditional or adding it inside the if (isDockerE2E)
block to avoid resource leaks.
Copilot uses AI. Check for mistakes.
template/frontend/package.json.jinja
Outdated
@@ -52,6 +52,7 @@ | |||
"happy-dom": "^17.4.4",{% endraw %}{% if frontend_uses_graphql %}{% raw %} | |||
"mock-apollo-client": "^1.3.1", | |||
"nuxt-codegen": "^1.3.1",{% endraw %}{% endif %}{% raw %} | |||
"playwright": "^1.52.0", | |||
"playwright-core": "^1.51.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The playwright
and playwright-core
dependencies are on different versions (1.52.0
vs 1.51.1
). Aligning these versions can prevent subtle compatibility issues.
"playwright-core": "^1.51.1", | |
"playwright-core": "^1.52.0", |
Copilot uses AI. Check for mistakes.
@@ -25,7 +25,7 @@ $env:Path = "C:\Users\runneradmin\.local\bin;$env:Path" | |||
|
|||
|
|||
$SCRIPT_DIR = Split-Path -Parent $MyInvocation.MyCommand.Definition | |||
$PROJECT_ROOT_DIR = Resolve-Path (Join-Path $SCRIPT_DIR "..") | |||
$PROJECT_ROOT_DIR = Resolve-Path (Join-Path $SCRIPT_DIR -ChildPath ".." | Join-Path -ChildPath ".." | Join-Path -ChildPath ".." | Join-Path -ChildPath "backend") # TODO: figure out a better approach than this hack way of specifying backend directory |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] This deeply nested Join-Path
chain is brittle. Consider computing the backend path relative to a known environment variable or using a configuration input to simplify resolution.
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR enables deploying the application as a standalone executable by integrating pyinstaller and adjusting configuration files, scripts, and workflows to support combined frontend/backend builds. Key changes include:
- Introducing a deploy_as_executable option in configuration files and conditional logic for CI and image creation.
- Updating various dependency versions and switching from shell scripts to Python-based tooling installation.
- Streamlining environment configurations and CI workflows to support both Python and Node setups.
Reviewed Changes
Copilot reviewed 65 out of 65 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
template/.devcontainer/envs.json.jinja | Added environment configuration with conditional backend inclusion. |
template/.devcontainer/devcontainer.json.jinja | Enabled shared settings for executable deployment based on new flag. |
template/.coveragerc | Removed entrypoint.py from coverage omissions. |
pyproject.toml | Pinned the pyright version to avoid known issues. |
extensions/context.py | Updated various dependency versions. |
copier.yml | Introduced deploy_as_executable flag and adjusted conditions for image deployment. |
README.md | Updated setup command to invoke the Python-based installation script. |
.github/workflows/pre-commit.yaml & ci.yaml | Modified workflow steps to install dependencies and pass Node version appropriately. |
.devcontainer/* & .github/actions/* | Replaced/deleted deprecated scripts and added Python alternatives for CI tooling. |
.copier-answers.yml | Updated commit identifier. |
copier.yml
Outdated
@@ -141,7 +150,7 @@ backend_image_name: | |||
type: str | |||
help: What is the name of the backend image? | |||
default: "{{ repo_name }}/backend" | |||
when: "{{ has_backend }}" | |||
when: "{{ has_backend and not deploy_as_executable}}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Consider adding a space before the closing curly brace for consistency (e.g., "{{ has_backend and not deploy_as_executable }}").
when: "{{ has_backend and not deploy_as_executable}}" | |
when: "{{ has_backend and not deploy_as_executable }}" |
Copilot uses AI. Check for mistakes.
with: | ||
python-version: ${{ matrix.python-version }} | ||
node-version: 22.14.0 # TODO: don't hardcode this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid hardcoding the Node version; consider parameterizing this value to enhance maintainability.
node-version: 22.14.0 # TODO: don't hardcode this | |
node-version: ${{ env.NODE_VERSION }} # Using parameterized Node.js version |
Copilot uses AI. Check for mistakes.
Link to Issue or Message thread
#21
Why is this change necessary?
Need to be able to deploy non-docker applications that have access to local computer
How does this change address the issue?
Creates option for a pyinstaller executable that serves a static frontend as part of it
What side effects does this change have?
None
How is this change tested?
Downstream repo using pyinstaller and one with a standard docker graphql backend
Other
Switched to just using standard playwright for E2E testing---created separate "compiled" testing that using the vitest fancy nuxt stuff
Optimized docker build layers
Added a
shutdown
route and moved all the API under/api
to not conflict with serving a static websiteRemoved some of the unnecessary
needs
from therequired-check
jobClose #21 along with creating a CLI for specifying things like port and log folder