Skip to content

Conversation

@Sheikah45
Copy link
Member

@Sheikah45 Sheikah45 commented Dec 23, 2025

Closes #31
Use tilt ci command to run tests of the infrastructure in an automated fashion.

tilt ci works by running the TiltFile specified and waiting for all the configured resources to start up. If any resource fails then the test fails. This takes into account all the health checks for the various services as well. More can be found at tilt.dev

Summary by CodeRabbit

  • New Features

    • Adds a Traefik namespace, improves startup ordering for Traefik, WordPress and the website, and applies website configuration overrides.
    • Registers an Icebreaker database and exposes DB_NAME.
    • Bumps Icebreaker service image to 1.2.0-RC4.
    • Adds resource requests (memory/cpu) for the user service container.
  • Bug Fixes

    • Fixes OAuth client initialization command argument continuation.
  • Chores

    • Adds a containerized "Checks" CI workflow running Tilt CI and Helm.
  • Documentation

    • Updates Test Data reference to an external location.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link

coderabbitai bot commented Dec 23, 2025

📝 Walkthrough

Walkthrough

Adds a GitHub Actions "Checks" workflow that runs Tilt CI in a docker/tilt container; introduces a traefik Namespace and updates Tilt resource dependencies and website rendering flow; registers an Icebreaker DB/config entry; bumps the faf-icebreaker image and adds resource requests to faf-user-service; fixes shell continuation in Hydra init script.

Changes

Cohort / File(s) Summary
CI workflow
\.github/workflows/checks.yml
New GitHub Actions workflow: triggers on push/PR to develop; runs in docker/tilt:latest; steps: checkout, create Kind via ctlptl, install Helm (azure/setup-helm), run tilt ci with a 5m timeout.
Tilt configuration
\Tiltfile``
Added traefik:namespace to namespaces objects; k8s_resource("traefik-setup", ...) now depends on ["namespaces"]; faf-website workload now depends on ["traefik","wordpress"]; website rendering switched to helm_with_build_cachepatch_configk8s_yaml.
Kubernetes namespaces
\cluster/namespaces.yaml``
Added Namespace resource named traefik and inserted document separator (---) between resources.
Hydra init template
\apps/ory-hydra/templates/init-clients.yaml``
Fixed multi-line hydra create oauth2-client shell command by adding trailing backslashes to args and an explicit terminating semicolon line.
Config & DB entries
\apps/faf-icebreaker/templates/config.yaml`, `infra/mariadb/values.yaml``
Added DB_NAME: "faf-icebreaker" to ConfigMap; registered Icebreaker DB in MariaDB databasesAndUsers with configMapRef: faf-icebreaker, secretRef: faf-icebreaker, and key mappings for DB/username/password.
Deployment image & resources
\apps/faf-icebreaker/templates/deployment.yaml`, `apps/faf-user-service/templates/deployment.yaml``
Bumped faf-icebreaker image tag 1.2.0-RC21.2.0-RC4; added container resource requests (memory: 2Gi, cpu: 1000m) to faf-user-service.
Docs
\README.MD``
Updated Test Data reference from local path /sql/test-data.sql to external GitHub URL (https://github.com/FAForever/db/blob/develop/test-data.sql).

Sequence Diagram(s)

sequenceDiagram
    autonumber
    participant GH as GitHub Actions
    participant Cont as Runner Container\n(docker/tilt:latest)
    participant CTL as ctlptl / Kind
    participant Helm as Helm (azure/setup-helm)
    participant Tilt as Tilt CI

    GH->>Cont: start workflow (checkout)
    Cont->>CTL: create Kind cluster via ctlptl
    CTL-->>Cont: cluster ready
    Cont->>Helm: install Helm
    Helm-->>Cont: ready
    Cont->>Tilt: run `tilt ci` (apply Tiltfile)
    Tilt-->>Cont: render/apply resources (namespaces, traefik, workloads)
    Cont->>GH: report workflow status

    rect rgba(220,238,255,0.6)
      note right of Tilt: Tiltfile now declares\n`traefik` namespace and\nupdated resource dependencies
    end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

Suggested reviewers

  • Brutus5000

Poem

"I hopped through YAML and CI tonight,
I added a namespace and fixed a backslash right,
I bumped an image, told Tilt to fly,
Helm nodded as clusters came alive,
— a rabbit's tiny dev delight 🥕"

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Out of Scope Changes check ⚠️ Warning Some changes appear unrelated to Tilt CI setup: updating faf-icebreaker deployment image version, adding DB configuration, updating faf-user-service resource requests, and updating website deployment flow dependencies. Review whether faf-icebreaker image update, database configuration changes, faf-user-service resource requests, and website deployment dependencies are necessary for Tilt CI testing or should be separated into distinct PRs.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Set up tilt CI for testing gitops-stack' directly and clearly summarizes the main change: adding Tilt CI integration to the gitops-stack repository.
Linked Issues check ✅ Passed The PR addresses issue #31 by implementing automated infrastructure testing via Tilt CI, fulfilling the requirement for CI/CD tests similar to the Postman collection approach used in faf-stack.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch tilt-ci

📜 Recent review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4f38cfe and 5f5a0cd.

📒 Files selected for processing (10)
  • .github/workflows/checks.yml
  • README.MD
  • Tiltfile
  • apps/faf-icebreaker/templates/config.yaml
  • apps/faf-icebreaker/templates/deployment.yaml
  • apps/faf-user-service/templates/deployment.yaml
  • apps/ory-hydra/templates/init-clients.yaml
  • cluster/namespaces.yaml
  • infra/mariadb/values.yaml
  • tilt/scripts/helm-with-cache.sh
🚧 Files skipped from review as they are similar to previous changes (5)
  • README.MD
  • apps/faf-icebreaker/templates/deployment.yaml
  • apps/faf-icebreaker/templates/config.yaml
  • infra/mariadb/values.yaml
  • .github/workflows/checks.yml
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: Sheikah45
Repo: FAForever/gitops-stack PR: 268
File: .github/workflows/checks.yml:9-14
Timestamp: 2025-12-23T18:21:33.542Z
Learning: The docker/tilt:latest image is a comprehensive CI image that includes tilt, ctlptl, Docker CLI, and related Kubernetes tooling, making it suitable for GitHub Actions container jobs that need to create Kind clusters.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: checks
  • GitHub Check: checks
🔇 Additional comments (6)
apps/faf-user-service/templates/deployment.yaml (1)

43-45: LGTM! Good addition of resource requests.

Adding resource requests alongside the existing limits follows Kubernetes best practices and helps the scheduler make informed placement decisions. The values (2Gi memory, 1000m CPU) provide reasonable baseline requirements while allowing headroom up to the limits.

apps/ory-hydra/templates/init-clients.yaml (1)

69-74: LGTM! Critical shell syntax fix.

The added backslash continuations and explicit semicolon terminator ensure the multi-line hydra create oauth2-client command is properly formed regardless of which conditional flags are rendered. Without these continuations, the command could be malformed if --token-endpoint-auth-method or --owner were the last flags added by the template.

cluster/namespaces.yaml (1)

16-22: LGTM! Clean namespace addition.

The new traefik namespace is properly formatted and aligns with the Tiltfile changes that reference traefik:namespace in the namespaces resource configuration.

Tiltfile (3)

159-159: LGTM! Consistent namespace registration.

Adding traefik:namespace to the namespaces resource aligns with the new Traefik namespace defined in cluster/namespaces.yaml.


185-185: LGTM! Correct dependency ordering.

Adding resource_deps=["namespaces"] ensures the Traefik namespace exists before attempting to set up Traefik resources, preventing race conditions during cluster initialization.


240-244: LGTM! Cleaner website deployment flow.

The refactored approach using helm_with_build_cache and patch_config is more consistent with other resource definitions in the Tiltfile. Adding the wordpress dependency ensures proper startup ordering for the website workload.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@Sheikah45 Sheikah45 marked this pull request as ready for review December 23, 2025 17:47
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between bdae64a and a47e004.

📒 Files selected for processing (1)
  • .github/worklows/checks.yml
🔇 Additional comments (4)
.github/worklows/checks.yml (4)

1-6: LGTM!

The workflow name and pull request trigger configuration for the develop branch are correct.


15-16: LGTM!

The checkout action version is current and appropriate.


18-19: LGTM - assuming Docker access is resolved.

The ctlptl command syntax is correct for creating a Kind cluster with a registry. This step will work once Docker daemon access is properly configured (as noted in the earlier comment).


21-22: LGTM!

The tilt ci command is appropriate for running automated Tilt tests in CI.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

♻️ Duplicate comments (1)
.github/workflows/checks.yml (1)

12-23: Critical: Container cannot access Docker daemon; ctlptl will fail.

This is the same critical issue previously identified. The workflow configuration has two blocking problems:

  1. No Docker daemon access: GitHub-hosted runners don't provide Docker socket access to container jobs by default. The ctlptl create cluster kind command at line 20 requires Docker to create the Kind cluster and will fail.

  2. Missing ctlptl: The docker/tilt:latest image is not a maintained multi-tool container and does not bundle ctlptl. The official Tilt distribution is via CLI binaries.

Recommended solution: Remove the container configuration and run directly on the ubuntu-latest runner, which has Docker pre-installed. Then explicitly install ctlptl and tilt:

🔎 Proposed fix
 jobs:
   checks:
-
     runs-on: ubuntu-latest
-    container:
-      image: docker/tilt:latest
-
+    
     steps:
       - uses: actions/checkout@v4
-
+      
+      - name: Install ctlptl
+        run: |
+          CTLPTL_VERSION="0.8.34"
+          curl -fsSL https://github.com/tilt-dev/ctlptl/releases/download/v${CTLPTL_VERSION}/ctlptl.${CTLPTL_VERSION}.linux.x86_64.tar.gz | sudo tar -xzv -C /usr/local/bin ctlptl
+      
+      - name: Install Tilt
+        run: curl -fsSL https://raw.githubusercontent.com/tilt-dev/tilt/master/scripts/install.sh | bash
+      
       - name: Create k8s Kind Cluster
         run: ctlptl create cluster kind --registry=ctlptl-registry
-
+      
       - name: Test Using Local Config
         run: tilt ci
-
-      

Verify the latest versions of ctlptl and tilt if needed:

What is the latest stable version of ctlptl?
What is the recommended installation method for Tilt CLI in CI environments?
🧹 Nitpick comments (1)
.github/workflows/checks.yml (1)

3-7: Consider aligning trigger branch filters.

The push trigger runs on all branches while pull_request is restricted to develop. This asymmetry means the workflow executes on pushes to any branch but only on PRs targeting develop.

If the intent is to test only develop-related changes, consider adding the same branch filter to push:

on:
  push:
    branches:
      - develop
  pull_request:
    branches:
      - develop
📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a47e004 and 572b200.

📒 Files selected for processing (2)
  • .github/workflows/checks.yml
  • tilt/scripts/helm-with-cache.sh

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
apps/ory-hydra/templates/init-clients.yaml (1)

68-73: Critical: Shell syntax error when tokenEndpointAuthMethod is set but owner is not.

The trailing backslash on Line 69 creates a shell syntax error when .tokenEndpointAuthMethod is defined but .owner is not. In this scenario, the rendered command ends with:

--token-endpoint-auth-method "value" \

The dangling backslash with no continuation line causes the shell to fail.

🔎 Proposed fix

Remove the trailing backslash from Line 69 since it's conditionally the last parameter:

                  {{- if .tokenEndpointAuthMethod }}
-                  --token-endpoint-auth-method "{{ .tokenEndpointAuthMethod }}" \
+                  --token-endpoint-auth-method "{{ .tokenEndpointAuthMethod }}"
                  {{- end }}

Alternatively, if you need the backslash for cases where .owner follows, you could add the backslash conditionally:

                  {{- if .tokenEndpointAuthMethod }}
-                  --token-endpoint-auth-method "{{ .tokenEndpointAuthMethod }}" \
+                  --token-endpoint-auth-method "{{ .tokenEndpointAuthMethod }}"{{- if .owner }} \{{- end }}
                  {{- end }}
📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 572b200 and e0a4c3b.

📒 Files selected for processing (4)
  • .github/workflows/checks.yml
  • Tiltfile
  • apps/ory-hydra/templates/init-clients.yaml
  • cluster/namespaces.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
  • .github/workflows/checks.yml
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: Sheikah45
Repo: FAForever/gitops-stack PR: 268
File: .github/workflows/checks.yml:9-14
Timestamp: 2025-12-23T18:21:25.226Z
Learning: The docker/tilt:latest image is a comprehensive CI image that includes tilt, ctlptl, Docker CLI, and related Kubernetes tooling, making it suitable for GitHub Actions container jobs that need to create Kind clusters.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: checks
  • GitHub Check: checks
🔇 Additional comments (3)
Tiltfile (2)

159-159: LGTM! Correctly tracks the new traefik namespace.

The addition of "traefik:namespace" properly registers the new namespace with Tilt's resource tracking, aligning with the namespace definition added in cluster/namespaces.yaml.


185-185: LGTM! Proper resource dependency ensures correct ordering.

Adding resource_deps=["namespaces"] ensures that traefik-setup waits for all namespaces (including the new traefik namespace) to be created before attempting to deploy Traefik resources. This prevents potential race conditions and follows the same dependency pattern used by other resources in the Tiltfile.

cluster/namespaces.yaml (1)

18-22: LGTM! Clean addition of the traefik namespace.

The new traefik Namespace is properly defined with correct YAML structure and delimiter. This aligns with the Tiltfile changes that reference and depend on this namespace, establishing a dedicated namespace for Traefik infrastructure components separate from application namespaces.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CI/CD testing strategy

2 participants