Conversation
Signed-off-by: Dmitrii Creed <creeed22@gmail.com>
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. ℹ️ You can also turn on project coverage checks and project coverage reporting on Pull Request comment Thanks for integrating Codecov - We've got you covered ☂️ |
There was a problem hiding this comment.
Pull request overview
This PR introduces a full “DevOps Info Service” implementation (Python/FastAPI + Java/Spring Boot), adds a GitHub Actions CI/CD workflow with testing/linting/security scan + Docker publishing, and substantially rewrites the course’s Lab 18 content from “4EVERLAND/IPFS” to “Reproducible Builds with Nix”.
Changes:
- Add Python FastAPI service with pytest coverage config, tests, Dockerfile, and CI workflow (tests/lint, Snyk scan, Docker build/push).
- Add Java Spring Boot bonus implementation with multi-stage Docker build and documentation.
- Replace Lab 18 curriculum content (and remove
labs/lab18/index.html) to focus on Nix reproducible builds; update root README lab topic text.
Reviewed changes
Copilot reviewed 35 out of 42 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| labs/lab18/index.html | Removes previously provided static landing page asset for old Lab 18 topic |
| labs/lab18.md | Rewrites Lab 18 to focus on Nix reproducible builds (tasks, rubric, troubleshooting) |
| app_python/app.py | Adds FastAPI app with / and /health endpoints + env-based config |
| app_python/requirements.txt | Adds Python runtime + dev/test dependencies |
| app_python/requirements-freeze.txt | Adds a dependency “freeze” snapshot (currently committed with problematic encoding) |
| app_python/pyproject.toml | Adds pytest + coverage configuration (coverage thresholds/reports) |
| app_python/tests/init.py | Initializes test package (contains an outdated note) |
| app_python/tests/test_app.py | Adds comprehensive unit tests for service endpoints |
| app_python/Dockerfile | Adds Docker image build for Python app (non-root user, slim base) |
| app_python/.dockerignore | Adds Docker build context exclusions |
| app_python/README.md | Adds usage/docs for running/testing/containerizing the Python service |
| app_python/docs/LAB01.md | Adds Lab 01 write-up/documentation for Python implementation |
| app_python/docs/LAB02.md | Adds Lab 02 Docker write-up for Python app |
| app_python/docs/LAB03.md | Adds Lab 03 CI/CD write-up and rationale |
| app_python/docs/screenshots/02-health-check.png | Adds documentation screenshot artifact |
| app_python/pycache/app.cpython-310.pyc | Adds Python bytecode artifact (should not be versioned) |
| app_java/pom.xml | Adds Maven project definition for Spring Boot app |
| app_java/src/main/resources/application.properties | Adds Spring Boot configuration (port/host/logging/actuator) |
| app_java/src/main/java/com/devops/infoservice/InfoServiceApplication.java | Adds Spring Boot entrypoint |
| app_java/src/main/java/com/devops/infoservice/controller/InfoController.java | Adds REST endpoints / and /health for Java app |
| app_java/src/main/java/com/devops/infoservice/model/ServiceResponse.java | Adds DTO for main response payload |
| app_java/src/main/java/com/devops/infoservice/model/ServiceInfo.java | Adds DTO for service metadata |
| app_java/src/main/java/com/devops/infoservice/model/SystemInfo.java | Adds DTO for system information |
| app_java/src/main/java/com/devops/infoservice/model/RuntimeInfo.java | Adds DTO for runtime/uptime info |
| app_java/src/main/java/com/devops/infoservice/model/RequestInfo.java | Adds DTO for request metadata |
| app_java/src/main/java/com/devops/infoservice/model/EndpointInfo.java | Adds DTO for endpoint listing |
| app_java/src/main/java/com/devops/infoservice/model/HealthResponse.java | Adds DTO for health response |
| app_java/Dockerfile | Adds multi-stage Docker build for Java app (builder + JRE runtime) |
| app_java/.gitignore | Adds Java/Maven ignore rules (but build output still committed elsewhere) |
| app_java/.dockerignore | Adds Docker build context exclusions for Java app |
| app_java/README.md | Adds run/build/docs for Java implementation |
| app_java/docs/LAB01.md | Adds Lab 01 bonus documentation for Java app |
| app_java/docs/LAB02.md | Adds Lab 02 bonus documentation for Java multi-stage build |
| app_java/docs/JAVA.md | Adds language/framework justification document |
| app_java/target/classes/application.properties | Commits build output under target/ (should not be versioned) |
| .github/workflows/python-ci.yml | Adds CI workflow: pytest+coverage, Ruff lint, Snyk scan, Docker build/push |
| .vscode/settings.json | Adds workspace settings for Java build configuration |
| README.md | Updates Lab 18 topic references to Nix reproducible builds |
Comments suppressed due to low confidence (1)
README.md:69
- Lab 18 is listed here as 20 pts and part of the exam alternative, but
labs/lab18.mdnow defines Lab 18 as 12 pts (and changes its scope substantially). Please reconcile the grading/points/exam-alternative text so it’s consistent across docs.
Don't want to take the exam? Complete **both** bonus labs:
| Lab | Topic | Points |
|-----|-------|--------|
| **Lab 17** | Fly.io Edge Deployment | 20 pts |
| **Lab 18** | Reproducible Builds with Nix | 20 pts |
**Requirements:**
- Complete both labs (17 + 18 = 40 pts, replaces exam)
- Minimum 16/20 on each lab
- Deadline: **1 week before exam date**
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| run: | | ||
| python -m pip install --upgrade pip | ||
| pip install -r requirements.txt | ||
| pip install ruff |
There was a problem hiding this comment.
ruff is already installed via requirements.txt (it’s listed there), so pip install ruff is redundant and can create version skew. Remove the extra install or pin/install ruff in exactly one place.
| pip install ruff |
| uses: actions/checkout@v4 | ||
|
|
||
| - name: Run Snyk to check for vulnerabilities | ||
| uses: snyk/actions/python@master |
There was a problem hiding this comment.
Using snyk/actions/python@master makes the workflow non-deterministic and can introduce breaking changes unexpectedly. Pin this action to a tagged release or a specific commit SHA for supply-chain safety and reproducible CI runs.
| uses: snyk/actions/python@master | |
| uses: snyk/actions/python@v1 |
| pytest | ||
| pytest-cov | ||
| httpx==0.28.1 | ||
| ruff No newline at end of file |
There was a problem hiding this comment.
Several dev/test tools are unpinned (pytest, pytest-cov, ruff). Since CI and docs reference specific versions, leaving these floating can cause unexpected CI breakage over time. Consider pinning these versions (or introducing a lockfile) to keep CI reproducible.
| # Lab 18 — Reproducible Builds with Nix | ||
|
|
||
|  | ||
|  | ||
|  | ||
|  | ||
|  | ||
|  |
There was a problem hiding this comment.
The PR title is "Lab03", but this change completely rewrites Lab 18 (topic, grading, deliverables). Please confirm this is intended for this PR, or split Lab 18 curriculum changes into a separate PR so the review scope matches the stated intent.
| spring.application.name=devops-info-service | ||
| application.version=1.0.0 | ||
|
|
||
| # Server configuration | ||
| server.port=${PORT:8080} | ||
| server.address=${HOST:0.0.0.0} | ||
|
|
||
| # Logging | ||
| logging.level.root=INFO | ||
| logging.level.com.devops.infoservice=DEBUG | ||
| logging.pattern.console=%d{yyyy-MM-dd HH:mm:ss} - %logger{36} - %msg%n | ||
|
|
||
| # Actuator endpoints | ||
| management.endpoints.web.exposure.include=health,info | ||
| management.endpoint.health.show-details=always |
There was a problem hiding this comment.
This looks like a build output under target/ and shouldn’t be checked into source control. Please remove app_java/target/** from the repo and ensure it’s ignored (root .gitignore or enforce via build tooling) so builds stay clean.
| spring.application.name=devops-info-service | |
| application.version=1.0.0 | |
| # Server configuration | |
| server.port=${PORT:8080} | |
| server.address=${HOST:0.0.0.0} | |
| # Logging | |
| logging.level.root=INFO | |
| logging.level.com.devops.infoservice=DEBUG | |
| logging.pattern.console=%d{yyyy-MM-dd HH:mm:ss} - %logger{36} - %msg%n | |
| # Actuator endpoints | |
| management.endpoints.web.exposure.include=health,info | |
| management.endpoint.health.show-details=always | |
| # This file resides under app_java/target/classes and is a build artifact. | |
| # It should not be version-controlled. Remove app_java/target/** from the repo | |
| # and add it to .gitignore or configure your build tooling accordingly. |
| @@ -0,0 +1 @@ | |||
| # Unit tests will be added in Lab 3 | |||
There was a problem hiding this comment.
This comment is now outdated because unit tests are already present in tests/. Update/remove it to avoid confusing readers.
| from datetime import datetime | ||
| import platform | ||
| import socket |
There was a problem hiding this comment.
platform and socket imports are unused in this test module, which adds noise and will be flagged by linters when stricter rules are enabled. Remove the unused imports to keep the test file clean.
| return { | ||
| 'hostname': socket.gethostname(), | ||
| 'platform': platform.system(), | ||
| 'platform_version': platform.version(), | ||
| 'architecture': platform.machine(), | ||
| 'cpu_count': os.cpu_count(), |
There was a problem hiding this comment.
os.cpu_count() can legally return None. Returning that value directly makes the API schema inconsistent (and your tests expect an int). Consider normalizing it to an integer (e.g., fallback to 0/1) before returning.
| return { | |
| 'hostname': socket.gethostname(), | |
| 'platform': platform.system(), | |
| 'platform_version': platform.version(), | |
| 'architecture': platform.machine(), | |
| 'cpu_count': os.cpu_count(), | |
| cpu_count = os.cpu_count() | |
| if cpu_count is None: | |
| cpu_count = 1 | |
| return { | |
| 'hostname': socket.gethostname(), | |
| 'platform': platform.system(), | |
| 'platform_version': platform.version(), | |
| 'architecture': platform.machine(), | |
| 'cpu_count': cpu_count, |
| 3. **Force an actual rebuild to prove reproducibility:** | ||
|
|
||
| ```bash | ||
| # First, find your build's store path | ||
| STORE_PATH=$(readlink result) | ||
| echo "Original store path: $STORE_PATH" | ||
|
|
||
| # Delete it from the Nix store | ||
| nix-store --delete $STORE_PATH | ||
|
|
||
| # Now rebuild (this forces actual compilation) | ||
| rm result | ||
| nix-build | ||
| readlink result | ||
| ``` |
There was a problem hiding this comment.
Instructing students to run nix-store --delete is risky because it can remove store paths that may still be referenced by other profiles/GC roots and can lead to a broken local environment. Prefer safer reproducibility checks like nix-build --check / nix build --rebuild (or explicitly explain GC roots and provide a safer alternative).
|
|
||
| ### 1.1 Non-Root User | ||
| **Practice:** Created a dedicated user (`appuser`) and switched to it using the `USER` directive. | ||
| **Why:** Running containers as root is a major security risk. If an attacker limits the container scope, they still potential access to the host as root. A non-root user limits the potential blast radius of a security compromise. |
There was a problem hiding this comment.
Grammar issue: “they still potential access” is incorrect. Consider rewriting as “they still potentially have access …” for clarity.
| **Why:** Running containers as root is a major security risk. If an attacker limits the container scope, they still potential access to the host as root. A non-root user limits the potential blast radius of a security compromise. | |
| **Why:** Running containers as root is a major security risk. If an attacker limits the container scope, they still potentially have access to the host as root. A non-root user limits the potential blast radius of a security compromise. |
No description provided.