Conversation
Signed-off-by: Assia AIT LAMALEM <assia.aitlamalem@gmail.com>
67e9c8d to
5cea276
Compare
razvand
left a comment
There was a problem hiding this comment.
Why are updates to the index.html and app-nginx?
For the actual workflow files, copy and adapt from existing workflow files, no need to start from scratch.
| @@ -1,6 +1,6 @@ | |||
| spec: v0.6 | |||
|
|
|||
| runtime: base-compat:latest | |||
| runtime: index.unikraft.io/official/nginx:latest | |||
There was a problem hiding this comment.
Hi @razvand,
Thanks for the review and the feedback! I understand the confusion regarding the extra changes. My goal was to update the example to ensure it uses the most robust and maintainable approach for the latest Nginx version.
…fy example scope as requested.
… file changes as requested.
razvand
left a comment
There was a problem hiding this comment.
You have an nginx-1.25/app-nginx` symbolic link or something weird. Remove it.
Use only two commit messages: one for moving the current nginx/ directory to nginx-1.25/ and another for adding the current nginx-1.27/ directory.
The .github/workflow/ directory is in the root directory of the repository, not a subdirectory of an example.
Write good commit messages. See the commit history, and use similar messages. Sign-off your commits.
|
Pls update the version of the existing nginx example instead |
There was a problem hiding this comment.
Pull request overview
Adds versioned Nginx 1.25 and 1.27 examples with KraftKit configuration, Dockerfiles, documentation, and CI workflows.
Changes:
- Adds root filesystems, configurations, Kraftfiles, and Dockerfiles for both versions.
- Adds stable and staging workflow configurations with build and curl checks.
- Removes the legacy Nginx page.
Reviewed changes
Copilot reviewed 12 out of 16 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Summary |
|---|---|
nginx/rootfs/wwwroot/index.html |
Removes the legacy page. Critical (1 vote): existing references still target the removed nginx/ directory. |
nginx-1.27/rootfs/wwwroot/index.html |
Adds example web content. No final findings. |
nginx-1.27/rootfs/etc/nginx/nginx.conf |
Adds Nginx configuration. No final findings. |
nginx-1.27/README.md |
Documents the example. Nit (4 votes): documented paths are stale. |
nginx-1.27/Kraftfile |
Defines the Nginx runtime. No final findings. |
nginx-1.27/Dockerfile |
Moderate (4 votes): uses the Nginx 1.25.3 image for the 1.27 example. |
nginx-1.27/.github/workflows/example-nginx-staging.yaml |
Critical (4 votes): nested workflow is not discovered; Critical (2 votes): uses nonexistent nginx path. |
nginx-1.27/.github/workflows/example-nginx-stable.yaml |
Critical (4 votes): nested workflow is not discovered; Critical (2 votes): uses nonexistent nginx path. |
nginx-1.25/rootfs/wwwroot/index.html |
Adds example web content. No final findings. |
nginx-1.25/rootfs/etc/nginx/nginx.conf |
Adds Nginx configuration. No final findings. |
nginx-1.25/README.md |
Nit (4 votes each): checkout and configuration paths are stale. |
nginx-1.25/Kraftfile |
Critical (4 votes): rootfs bypasses the Dockerfile and omits the Nginx binary and runtime libraries. |
nginx-1.25/Dockerfile |
Defines the container build. No final findings. |
.github/workflows/example-nginx-staging.yaml |
Critical (4 votes): uses nonexistent nginx path; Moderate (4 votes): filters on staging instead of main. |
.github/workflows/example-nginx-stable.yaml |
Critical (4 votes): uses nonexistent nginx path; Moderate (4 votes): filters on add-nginx-latest instead of the stable branch. |
Suppressed comments (11)
.github/workflows/example-nginx-stable.yaml:27
kraft buildonly creates the unikernel image; it does not start a listener on the GitHub runner. Since this job never runs the built image,curl http://localhost:8080cannot test Nginx and will fail with connection refused. Add a run/deploy step and wait for readiness before curling.
kraft build
- name: Test Nginx
run: |
curl http://localhost:8080
.github/workflows/example-nginx-stable.yaml:27
- Even after a server is launched, plain
curlexits successfully for HTTP 4xx/5xx responses. This test can therefore pass on an Nginx error page instead of verifying a successful response; use--fail-with-body(and assert the expected page if that is the contract).
curl http://localhost:8080
.github/workflows/example-nginx-stable.yaml:7
- The previous workflow had a weekday schedule and manual dispatch, but both triggers were removed here. Since this example consumes mutable
:latestruntime tags, upstream runtime changes will no longer be tested unless repository code changes; restore a scheduled/manual trigger if the workflow is meant to validate the latest image continuously.
on:
push:
branches: [add-nginx-latest]
pull_request:
branches: [add-nginx-latest]
.github/workflows/example-nginx-staging.yaml:27
kraft buildonly creates the unikernel image; it does not start a listener on the GitHub runner. Since this job never runs the built image,curl http://localhost:8080cannot test Nginx and will fail with connection refused. Add a run/deploy step and wait for readiness before curling.
kraft build
- name: Test Nginx
run: |
curl http://localhost:8080
.github/workflows/example-nginx-staging.yaml:27
- Even after a server is launched, plain
curlexits successfully for HTTP 4xx/5xx responses. This test can therefore pass on an Nginx error page instead of verifying a successful response; use--fail-with-body(and assert the expected page if that is the contract).
curl http://localhost:8080
.github/workflows/example-nginx-staging.yaml:7
- The previous workflow had a weekday schedule and manual dispatch, but both triggers were removed here. Since this example consumes mutable
:latestruntime tags, upstream runtime changes will no longer be tested unless repository code changes; restore a scheduled/manual trigger if the workflow is meant to validate the latest image continuously.
on:
push:
branches: [staging]
pull_request:
branches: [staging]
nginx-1.27/.github/workflows/example-nginx-stable.yaml:27
- If this workflow is relocated so GitHub can execute it,
kraft buildstill only creates an image and never starts the service, so the following localhost curl has no listener to contact. Add a run/deploy and readiness step before this test.
kraft build
- name: Test Nginx
run: |
curl http://localhost:8080
nginx-1.27/.github/workflows/example-nginx-staging.yaml:27
- If this workflow is relocated so GitHub can execute it,
kraft buildstill only creates an image and never starts the service, so the following localhost curl has no listener to contact. Add a run/deploy and readiness step before this test.
kraft build
- name: Test Nginx
run: |
curl http://localhost:8080
nginx-1.27/README.md:88
- This file does not exist at the documented path: the configuration added by this PR is
rootfs/etc/nginx/nginx.conf. Users following this customization list will edit the wrong location.
* `rootfs/conf/nginx.conf`: the Nginx configuration file
nginx-1.27/README.md:109
- The customization instructions repeat a nonexistent
rootfs/conf/nginx.confpath. The actual configuration file isrootfs/etc/nginx/nginx.conf, so this instruction cannot be followed.
If required, you can also customize the configuration of Nginx in `rootfs/conf/nginx.conf`.
nginx/rootfs/wwwroot/index.html:1
- This deletion removes the only tracked content under
nginx/, but the repository README still links tomain/nginxand the active workflows still try tocd nginx. After merge those references point to a dead example; update them to the intended versioned example or retain a compatibilitynginx/directory.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| - name: Build Nginx Example | ||
| run: | | ||
| set -xe; | ||
| cd nginx |
| branches: [add-nginx-latest] | ||
| pull_request: | ||
| types: [opened, synchronize, reopened] | ||
| branches: [main] | ||
| paths: | ||
| - '.github/workflows/example-nginx-stable.yaml' | ||
| - 'nginx/**' | ||
| - '!nginx/README.md' | ||
|
|
||
| schedule: | ||
| - cron: '0 16 * * 1-5' | ||
|
|
||
| # Automatically cancel in-progress actions on the same branch | ||
| concurrency: | ||
| group: ${{ github.workflow }}-${{ github.event_name == 'pull_request_target' && github.head_ref || github.ref }} | ||
| cancel-in-progress: true | ||
|
|
||
| env: | ||
| UKC_METRO: "https://api.${{ vars.UKC_METRO_STABLE }}.unikraft.cloud/v1" | ||
| UKC_TOKEN: ${{ secrets.UKC_TOKEN }} | ||
| KRAFTKIT_NO_CHECK_UPDATES: true | ||
| KRAFTKIT_LOG_LEVEL: debug | ||
| branches: [add-nginx-latest] |
| - name: Build Nginx Example | ||
| run: | | ||
| set -xe; | ||
| cd nginx |
| branches: [staging] | ||
| pull_request: | ||
| types: [opened, synchronize, reopened] | ||
| branches: [main] | ||
| paths: | ||
| - '.github/workflows/example-nginx-staging.yaml' | ||
| - 'nginx/**' | ||
| - '!nginx/README.md' | ||
|
|
||
| schedule: | ||
| - cron: '0 16 * * 1-5' | ||
|
|
||
| # Automatically cancel in-progress actions on the same branch | ||
| concurrency: | ||
| group: ${{ github.workflow }}-${{ github.event_name == 'pull_request_target' && github.head_ref || github.ref }} | ||
| cancel-in-progress: true | ||
|
|
||
| env: | ||
| UKC_METRO: "https://api.${{ vars.UKC_METRO_STAGING }}.unikraft.cloud/v1" | ||
| UKC_TOKEN: ${{ secrets.UKC_TOKEN }} | ||
| KRAFTKIT_NO_CHECK_UPDATES: true | ||
| KRAFTKIT_LOG_LEVEL: debug | ||
| branches: [staging] |
| @@ -0,0 +1,27 @@ | |||
| name: Build and Test Nginx Example - Stable | |||
|
|
||
| - name: Build Nginx Example | ||
| run: | | ||
| cd nginx |
| @@ -0,0 +1,27 @@ | |||
| name: Build and Test Nginx Example - Staging | |||
|
|
||
| - name: Build Nginx Example | ||
| run: | | ||
| cd nginx |
| @@ -0,0 +1,33 @@ | |||
| FROM nginx:1.25.3-bookworm AS build | |||
| 2. Clone the [`examples` repository](https://github.com/unikraft-cloud/examples) and `cd` into the `examples/nginx/` directory: | ||
|
|
||
| ```bash | ||
| git clone https://github.com/unikraft-cloud/examples | ||
| cd examples/nginx/ |
This PR adds GitHub Actions workflows for building and testing the latest Nginx example:
The workflows build the Nginx example using KraftKit and verify it with a curl request to localhost:8080.