Skip to content

Conversation

@mfleader
Copy link
Collaborator

@mfleader mfleader commented Jun 5, 2025

Switched the controller tests over to use envtest to create a controller runtime client instead of using the fake client.

Why?

The fake client doesn't support Server-Side Apply (SSA), which we'll need for the Kustomize overlay integration coming up. envtest gives us a real (mini) Kubernetes API.

What changed:

  • The test now runs against a Kubernetes API thanks to envtest. This means we had to:
    • Get envtest up and running.
    • Swap out the controller runtime client test double for a real client.
    • Made sure the tests play nice with KUBEBUILDER_ASSETS so envtest can find the k8s binaries.
  • Sprinkled in some comments to explain the envtest-specific bits.
  • Important: The actual tests for what the Deployment and PVC do haven't changed.

Heads up for next steps (separate PRs):

  • Updating reconcilePVC to use Kustomize with those dynamic overlays.
  • Switching PVC management to proper SSA (client.Patch(..., client.Apply, ...)), which these new envtest tests will be able to handle.

@mfleader mfleader self-assigned this Jun 5, 2025
@mfleader mfleader added the enhancement New feature or request label Jun 5, 2025
@mfleader mfleader changed the title adapt controller tests to use envtest for eventual ssa integration Migrate PVC controller tests to envtest for future SSA compatibility Jun 5, 2025
@mfleader mfleader marked this pull request as ready for review June 5, 2025 17:04
@mfleader mfleader force-pushed the controller-envtest branch 2 times, most recently from 9af4828 to e2edf84 Compare June 5, 2025 17:55
@mfleader mfleader requested review from VaishnaviHire, leseb and rhuss June 5, 2025 20:32
Signed-off-by: Matthew F Leader <mleader@redhat.com>

add comment on adding scheme and rename client

Signed-off-by: Matthew F Leader <mleader@redhat.com>

shorten comment

Signed-off-by: Matthew F Leader <mleader@redhat.com>

comment

Signed-off-by: Matthew F Leader <mleader@redhat.com>

fix comment

Signed-off-by: Matthew F Leader <mleader@redhat.com>
@mfleader mfleader force-pushed the controller-envtest branch from ca2e8ec to 5dda57f Compare June 5, 2025 21:55
@mfleader mfleader changed the title Migrate PVC controller tests to envtest for future SSA compatibility Migrate controller tests to envtest for future SSA compatibility Jun 6, 2025
@mergify
Copy link

mergify bot commented Jun 6, 2025

This pull request has merge conflicts that must be resolved before it can be merged. @mfleader please rebase it. https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

@mergify mergify bot added the needs-rebase label Jun 6, 2025
Signed-off-by: Matthew F Leader <mleader@redhat.com>
@mfleader mfleader marked this pull request as draft June 6, 2025 13:52
mfleader added 2 commits June 6, 2025 10:31
Signed-off-by: Matthew F Leader <mleader@redhat.com>
Signed-off-by: Matthew F Leader <mleader@redhat.com>
@mergify mergify bot removed the needs-rebase label Jun 6, 2025
@mfleader mfleader marked this pull request as ready for review June 6, 2025 14:41
@VaishnaviHire
Copy link
Collaborator

VaishnaviHire commented Jun 9, 2025

LGTM

I wonder though if we are now duplicating tests with https://github.com/llamastack/llama-stack-k8s-operator/tree/main/tests/e2e that run against a cluster

@mfleader
Copy link
Collaborator Author

mfleader commented Jun 10, 2025

LGTM

I wonder though if we are now duplicating tests with https://github.com/llamastack/llama-stack-k8s-operator/tree/main/tests/e2e that run against a cluster

That's definitely a good spot for tidying. It would be good to minimize the quantity of tests that require an instantiated cluster. I think it's worth creating an issue.

@mfleader mfleader removed the request for review from rhuss June 10, 2025 14:20
Copy link
Collaborator

@leseb leseb left a comment

Choose a reason for hiding this comment

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

Nice work!

Signed-off-by: Matthew F Leader <mleader@redhat.com>
@mfleader mfleader removed the request for review from VaishnaviHire June 10, 2025 20:43
@mergify mergify bot merged commit 48e36b6 into llamastack:main Jun 10, 2025
6 checks passed
VaishnaviHire pushed a commit to VaishnaviHire/llama-stack-k8s-operator that referenced this pull request Jun 18, 2025
…amastack#45)

Switched the controller tests over to use [envtest](https://pkg.go.dev/sigs.k8s.io/controller-runtime/pkg/envtest) to create a controller runtime client instead of using the [fake client](https://pkg.go.dev/sigs.k8s.io/controller-runtime/pkg/client/fake).

### Why?

The fake client doesn't support Server-Side Apply (SSA), which we'll need for the Kustomize overlay integration coming up. `envtest` gives us a real (mini) Kubernetes API.

### What changed:

*   The test now runs against a Kubernetes API thanks to `envtest`. This means we had to:
    *   Get `envtest` up and running.
    *   Swap out the controller runtime client test double for a real client.
    *   Made sure the tests play nice with `KUBEBUILDER_ASSETS` so `envtest` can find the k8s binaries.
*   Sprinkled in some comments to explain the `envtest`-specific bits.
*   **Important:** The actual tests for what the Deployment and PVC do haven't changed.

### Heads up for next steps (separate PRs):

*   Updating `reconcilePVC` to use Kustomize with those dynamic overlays.
*   Switching PVC management to proper SSA (`client.Patch(..., client.Apply, ...)`), which these new `envtest` tests will be able to handle.


Approved-by: leseb
@mfleader mfleader deleted the controller-envtest branch July 23, 2025 14:33
VaishnaviHire pushed a commit to VaishnaviHire/llama-stack-k8s-operator that referenced this pull request Sep 4, 2025
…f06ae0 (llamastack#45)

Signed-off-by: konflux-internal-p02 <170854209+konflux-internal-p02[bot]@users.noreply.github.com>
Co-authored-by: konflux-internal-p02[bot] <170854209+konflux-internal-p02[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants