Add /services endpoint for dynamic workspace service discovery#52
Add /services endpoint for dynamic workspace service discovery#52prasadtalasila merged 14 commits intomainfrom
Conversation
- Create admin service with Poetry project structure - Add FastAPI application with /services endpoint - Configure nginx to route /services to admin service - Update custom_startup.sh to start admin service - Add comprehensive tests for the service Co-authored-by: prasadtalasila <9206466+prasadtalasila@users.noreply.github.com>
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
- Fix FastAPI response type hints for root endpoint - Add service discovery section to README - Create detailed DOCUMENTATION.md for admin service - Verify all tests pass (8/8 passing) - Verify service works locally with correct MAIN_USER substitution Co-authored-by: prasadtalasila <9206466+prasadtalasila@users.noreply.github.com>
- Add .pytest_cache/ to .gitignore - Add poetry.lock to .gitignore (project-specific lock files shouldn't be committed for libraries) Co-authored-by: prasadtalasila <9206466+prasadtalasila@users.noreply.github.com>
|
@copilot fix the following problems. Problem-1: #10 375.5 It will add the
|
Problem-1: Fixed installation script path from ${INST_DIR}/../../admin to ${INST_DIR}/../admin
Problem-2: Removed whitespace and unused imports (os, Path) from Python files
Problem-3: Added pylint ^4.0.4 and pytest-cov ^7.0.0 to dev dependencies
Problem-4: Removed unused pytest-asyncio dependency
Improvement-5: Added CLI interface with argparse, making service runnable as workspace-admin command
- Updated DOCUMENTATION.md and README.md with pylint, pytest-cov, and CLI usage examples
- CLI supports --list-services, --host, --port, --reload, and --help options
- Service now works like a system utility similar to glances
Co-authored-by: prasadtalasila <9206466+prasadtalasila@users.noreply.github.com>
... All issues fixed in commit 940d813: Problem-1 (Docker build): Fixed path in install_admin.sh from Problem-2 (Linting): Removed all trailing whitespace and unused imports (os, Path) from Python files Problem-3 (Dependencies): Added pylint ^4.0.4 and pytest-cov ^7.0.0 with documentation for usage Problem-4 (pytest-asyncio): Removed unused pytest-asyncio dependency Improvement-5 (CLI): Added full CLI interface with argparse:
All tests pass (8/8) and service works as a standalone CLI utility similar to glances. |
…l+pipx installation Problem-1: Fixed all linting issues - Moved argparse and uvicorn imports to top level - Renamed unused service_id to _service_id - Fixed f-string without interpolation - Fixed test fixture warnings by renaming and proper usage - Fixed all flake8 whitespace and line length issues Problem-2: Increased test coverage from 50% to 84% - Added 6 new tests for CLI functionality and edge cases - All 14 tests pass Problem-3: Added locale prevention in apt-get - Added DEBIAN_FRONTEND=noninteractive and --no-install-recommends Problem-4 & 5: Changed from poetry run to wheel package + pipx - Updated install_admin.sh to build wheel with poetry build - Install package using pipx - Updated custom_startup.sh to use workspace-admin command - Fixed copy path to ensure pyproject.toml is found Co-authored-by: prasadtalasila <9206466+prasadtalasila@users.noreply.github.com>
- Use [[ ]] instead of [ ] for tests - Add braces around variable references - Add || true to find command to handle potential errors Co-authored-by: prasadtalasila <9206466+prasadtalasila@users.noreply.github.com>
prasadtalasila
changed the title
There was a problem hiding this comment.
Pull request overview
Adds a new “admin” FastAPI service to enable dynamic workspace service discovery via an nginx-proxied /services endpoint, and integrates it into the container build/startup flow.
Changes:
- Introduces a new FastAPI admin service (with CLI) + service template JSON and extensive pytest coverage.
- Routes
/{MAIN_USER}/servicesthrough nginx to the admin service and starts/monitors the admin process at container startup. - Updates the Docker build to include/install the admin service and adjusts several install scripts for noninteractive apt usage.
Reviewed changes
Copilot reviewed 19 out of 21 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| workspaces/src/startup/nginx.conf | Adds upstream + proxy routing for the admin /services endpoint |
| workspaces/src/startup/custom_startup.sh | Starts and monitors the admin service process |
| workspaces/src/startup/configure_nginx.py | Substitutes {ADMIN_SERVER_PORT} into nginx config |
| workspaces/src/install/nginx/install_nginx.sh | Uses noninteractive apt behavior |
| workspaces/src/install/jupyter/install_jupyter.sh | Uses noninteractive apt behavior |
| workspaces/src/install/firefox/install_firefox.sh | Uses noninteractive apt behavior |
| workspaces/src/install/admin/install_admin.sh | New installer for admin service (Poetry build + pipx install) |
| workspaces/src/admin/src/admin/main.py | New FastAPI app + CLI entrypoint implementing /services |
| workspaces/src/admin/src/admin/services_template.json | Template defining discoverable services and endpoints |
| workspaces/src/admin/tests/test_main.py | New unit tests covering endpoints and CLI behavior |
| workspaces/src/admin/tests/init.py | Declares tests as a package |
| workspaces/src/admin/src/admin/init.py | Declares admin as a package |
| workspaces/src/admin/pyproject.toml | Poetry project definition + console script |
| workspaces/src/admin/poetry.lock | Dependency lockfile for the admin service |
| workspaces/src/admin/README.md | Admin service usage documentation |
| workspaces/src/admin/DOCUMENTATION.md | Detailed admin service documentation |
| workspaces/Dockerfile.ubuntu.noble.gnome | Copies/admin installs admin service during image build |
| README.md | Documents the new /services endpoint usage |
| CLAUDE.md | Updates repository guidance including admin service details |
| .gitignore | Adds ignores for pytest/poetry/build artifacts |
| .github/copilot-instructions.md | Adds admin service-specific contributor guidance |
| admin_server_port = os.getenv("ADMIN_SERVER_PORT") | ||
| call( | ||
| "sed -i 's@{ADMIN_SERVER_PORT}@" | ||
| + admin_server_port | ||
| + "@g' " | ||
| + NGINX_FILE, | ||
| shell=True | ||
| ) |
There was a problem hiding this comment.
admin_server_port = os.getenv("ADMIN_SERVER_PORT") can be None, which will raise a TypeError when concatenated into the sed command. If this script is run in an environment where ADMIN_SERVER_PORT isn’t set, nginx config generation will fail. Consider providing a default (e.g., "8091") or adding an explicit check with a clear error message before building the sed command.
| The service is automatically started when the workspace container starts. | ||
| It runs on port 8091 and is accessible via the nginx reverse proxy at `/services`. |
There was a problem hiding this comment.
This claims the service is accessible via nginx at /services, but the nginx config in this PR routes the endpoint under the workspace base path ({WORKSPACE_BASE_URL_DECODED}/services), which becomes /{MAIN_USER}/services (e.g., /user1/services). Updating this line will avoid sending users to a non-working URL.
| pipx install "${WHEEL_FILE}" | ||
| pipx ensurepath | ||
| source ~/.bashrc | ||
|
|
||
| # Verify installation | ||
| if ! command -v workspace-admin &> /dev/null; then | ||
| echo "Error: workspace-admin command not found after installation" | ||
| exit 1 |
There was a problem hiding this comment.
pipx ensurepath/source ~/.bashrc here won’t make workspace-admin available at container runtime because this script runs in a subprocess during docker build (environment changes won’t persist into later layers), and /tmp/.docker_set_envs captures the build-time PATH after this script finishes. This can lead to workspace-admin: command not found when custom_startup.sh runs. Prefer installing the CLI into a global location on PATH (e.g., symlink/copy the entrypoint into /usr/local/bin, or set an ENV PATH=... in the Dockerfile before /tmp/.docker_set_envs is generated) and drop the .bashrc sourcing.
| if [[ -n "${path_prefix}" ]]; then | ||
| workspace-admin --host 0.0.0.0 --port "${ADMIN_SERVER_PORT}" --path-prefix "${path_prefix}" & | ||
| else | ||
| workspace-admin --host 0.0.0.0 --port "${ADMIN_SERVER_PORT}" & |
There was a problem hiding this comment.
start_admin_server assumes workspace-admin is on PATH. With the current pipx-based install, the binary may land in ~/.local/bin and not be present in the runtime PATH (especially since dtaas_shim.sh restores env vars from /tmp/.docker_set_envs). Consider invoking it via an absolute path (e.g., ${HOME}/.local/bin/workspace-admin) or ensuring PATH is explicitly updated during image build/startup before relying on this command.
| if [[ -n "${path_prefix}" ]]; then | |
| workspace-admin --host 0.0.0.0 --port "${ADMIN_SERVER_PORT}" --path-prefix "${path_prefix}" & | |
| else | |
| workspace-admin --host 0.0.0.0 --port "${ADMIN_SERVER_PORT}" & | |
| local admin_cmd="${HOME}/.local/bin/workspace-admin" | |
| if [[ ! -x "${admin_cmd}" ]]; then | |
| # Fallback to PATH-resolved command if the expected absolute path is not executable. | |
| admin_cmd="workspace-admin" | |
| fi | |
| if [[ -n "${path_prefix}" ]]; then | |
| "${admin_cmd}" --host 0.0.0.0 --port "${ADMIN_SERVER_PORT}" --path-prefix "${path_prefix}" & | |
| else | |
| "${admin_cmd}" --host 0.0.0.0 --port "${ADMIN_SERVER_PORT}" & |
| default=os.getenv("PATH_PREFIX", "dtaas-user"), | ||
| help="Path prefix for API routes (e.g., 'dtaas-user' for routes at /dtaas-user/services)" |
There was a problem hiding this comment.
The CLI default for --path-prefix is dtaas-user, which means running workspace-admin with no args will mount routes under /dtaas-user/... rather than at /services//health. This contradicts the README examples in this PR (which show workspace-admin without requiring a prefix) and makes the “optional” prefix effectively mandatory. Defaulting --path-prefix to an empty string (and only enabling prefix when explicitly provided or when PATH_PREFIX is set) would align behavior and docs.
| default=os.getenv("PATH_PREFIX", "dtaas-user"), | |
| help="Path prefix for API routes (e.g., 'dtaas-user' for routes at /dtaas-user/services)" | |
| default=os.getenv("PATH_PREFIX", ""), | |
| help=( | |
| "Path prefix for API routes. " | |
| "Defaults to no prefix (routes at /services) unless PATH_PREFIX is set " | |
| "(e.g., 'dtaas-user' for routes at /dtaas-user/services)." | |
| ) |
| Returns: | ||
| JSONResponse containing service information. | ||
| """ | ||
| services = load_services(os.environ["PATH_PREFIX"] if "PATH_PREFIX" in os.environ else "") |
There was a problem hiding this comment.
create_app(path_prefix=...) only uses path_prefix for routing, but /services content substitution reads PATH_PREFIX from the environment instead. If the app is created with a prefix (e.g., in tests or when embedded) but PATH_PREFIX isn’t set, service endpoints won’t be prefixed even though the routes are. Consider capturing the cleaned path_prefix in the route handler and passing it into load_services(...) (or setting PATH_PREFIX consistently when path_prefix is provided).
| services = load_services(os.environ["PATH_PREFIX"] if "PATH_PREFIX" in os.environ else "") | |
| effective_prefix = os.environ.get("PATH_PREFIX", path_prefix) | |
| services = load_services(effective_prefix) |
| "desktop": { | ||
| "name": "Desktop", | ||
| "description": "Virtual Desktop Environment", | ||
| "endpoint": "tools/vnc?path={path-prefix}tools%2Fvnc%2Fwebsockify" |
There was a problem hiding this comment.
The documented example response has an incorrect VNC endpoint template: it’s missing the %2F separator after the path prefix and uses {path-prefix} while the implementation/template uses {PATH_PREFIX}. This will mislead consumers trying to construct the VNC URL. Update the example to match services_template.json and the actual substitution behavior.
| "endpoint": "tools/vnc?path={path-prefix}tools%2Fvnc%2Fwebsockify" | |
| "endpoint": "tools/vnc?path={PATH_PREFIX}%2Ftools%2Fvnc%2Fwebsockify" |
|
@DisasterlyDisco This PR is complete. Please review the same. If it is working as expected, then create a short commit message and merge the same. Thanks. |
| location ~* "^{WORKSPACE_BASE_URL_DECODED}/services" { | ||
| proxy_http_version 1.1; | ||
| proxy_set_header Host $host; | ||
| proxy_set_header X-Real-IP $remote_addr; | ||
| proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for; | ||
| proxy_set_header X-Forwarded-Proto $scheme; | ||
|
|
||
| proxy_pass http://admin; | ||
| } |
There was a problem hiding this comment.
The /services nginx location uses a case-insensitive regex (~*) without anchoring the end of the path. After placeholder substitution, this could unintentionally match additional paths beyond the intended endpoint (e.g., /user1/servicesXYZ). Since {WORKSPACE_BASE_URL_DECODED} is replaced before nginx starts, consider using an exact or prefix match location (e.g., location = …/services or location ^~ …/services) or at least anchor the regex to /services$//services(?:/|$).
| # Install Poetry and pipx | ||
| DEBIAN_FRONTEND=noninteractive apt-get install -y --no-install-recommends \ | ||
| python3-poetry \ | ||
| pipx | ||
|
|
||
| poetry --version | ||
| pipx --version |
There was a problem hiding this comment.
workspaces/src/admin/poetry.lock is generated by Poetry 2.3.2 (lock-version 2.1), but this install script pulls Poetry from the OS package (python3-poetry). If the distro package provides an older Poetry major version, poetry install/build can fail due to lockfile format incompatibility.
To make the Docker build reproducible, consider installing a known-compatible Poetry version (e.g., via install.python-poetry.org with an explicit version) or regenerating the lock file with the same Poetry version that will be used in the image build.
| echo "Installing wheel: ${WHEEL_FILE}" | ||
| pipx install "${WHEEL_FILE}" | ||
| pipx ensurepath | ||
| # shellcheck disable=SC1090 | ||
| source ~/.bashrc | ||
|
|
||
| # Verify installation | ||
| if ! command -v workspace-admin &> /dev/null; then | ||
| echo "Error: workspace-admin command not found after installation" | ||
| exit 1 |
There was a problem hiding this comment.
pipx install is executed as root during the image build, so the generated workspace-admin entrypoint will typically be placed under root’s pipx bin dir (e.g., /root/.local/bin). At runtime, dtaas_shim.sh switches to MAIN_USER before running custom_startup.sh, so workspace-admin may not be on the PATH and the admin server can fail to start.
Consider installing the CLI into a global bin dir (e.g., set PIPX_HOME/PIPX_BIN_DIR to a system location like /opt/pipx + /usr/local/bin, or use pipx install --global), or otherwise ensure the runtime user’s PATH includes the pipx bin directory.
| DEBIAN_FRONTEND=noninteractive apt-get install -y --no-install-recommends \ | ||
| python3-poetry \ | ||
| pipx |
There was a problem hiding this comment.
This script runs apt-get install without an apt-get update in the same layer. Depending on the base image state, this can fail with “Unable to locate package …”. Align with the other install scripts by running apt-get update before installing packages (and ideally cleaning /var/lib/apt/lists/* afterwards).
| DEBIAN_FRONTEND=noninteractive apt-get install -y --no-install-recommends \ | |
| python3-poetry \ | |
| pipx | |
| export DEBIAN_FRONTEND=noninteractive | |
| apt-get update | |
| apt-get install -y --no-install-recommends \ | |
| python3-poetry \ | |
| pipx | |
| rm -rf /var/lib/apt/lists/* |
| @router.get("/services") | ||
| async def get_services() -> JSONResponse: | ||
| """ | ||
| Get list of available workspace services. | ||
|
|
||
| Returns: | ||
| JSONResponse containing service information. | ||
| """ | ||
| services = load_services(os.environ["PATH_PREFIX"] if "PATH_PREFIX" in os.environ else "") | ||
| return JSONResponse(content=services) |
There was a problem hiding this comment.
get_services() uses PATH_PREFIX from the process environment to populate {PATH_PREFIX} substitutions, but route prefixing is controlled by the path_prefix passed into create_app(). This can lead to inconsistent behavior (e.g., app mounted under /user1 but the returned service endpoints still substitute an empty prefix when PATH_PREFIX isn’t set).
Prefer deriving the substitution prefix from create_app()’s configured prefix (e.g., close over the cleaned prefix and pass it to load_services()), rather than relying on a separate environment variable.
|
@DisasterlyDisco another PR on DTaaS depends on this PR. Hence this PR is being merged. |
Added two comprehensive markdown files for GitHub issue creation: - issue1_unresolved_comments.md: 37 unresolved review comments from merged PRs (PRs #52, #43, #16, #10, #8, #6) organized by PR with direct links, file locations, and recommendations - issue2_potential_improvements.md: 17 potential improvements identified through codebase analysis, organized by priority (Critical, High, Medium, Low) with detailed explanations, code examples, and implementation guidance Both files are placed in the repository root for easy access and can be used to create GitHub issues for tracking these items. Co-authored-by: prasadtalasila <9206466+prasadtalasila@users.noreply.github.com>
Admin service had linting errors, insufficient test coverage (50%), and used poetry runtime for service execution. Docker build also failed due to incorrect copy path for pyproject.toml.
Changes
Linting fixes
argparseanduvicornimports to module top levelservice_idto_service_idclient→test_client,mock_main_user→setup_mock_main_user)Test coverage: 50% → 84%
--list-services,--version), JSON structure validation, endpoint content-type checks, edge casesPackaging model change: poetry run → wheel + pipx
install_admin.sh:
custom_startup.sh:
Eliminates poetry runtime dependency, provides system-wide CLI.
Docker build fix
${INST_DIR}/../admin/*(was${INST_DIR}/admin)Locale prevention
DEBIAN_FRONTEND=noninteractive --no-install-recommendsto apt-get in install_admin.sh💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.