Conversation
- Added comprehensive HTTP client tests with proper organization - Implemented authentication testing for Prefect Cloud, self-hosted, and empty API keys - Added error handling tests for network failures, context cancellation, and invalid responses - Enhanced test coverage for deployment controller error scenarios - Added API key handling tests from various sources (secrets, configmaps, direct values) - Fixed Makefile to automatically exclude generated code from coverage - Updated AGENTS.md with better documentation structure and portforward testing guidance - Improved overall test consistency across all controllers - Fixed linting issues with error handling and unused functions 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
This change reduces log noise by demoting fine-grained operational details to DEBUG level while keeping important milestones at INFO level: - HTTP API calls in Prefect client (16 log statements) - Routine reconciliation start messages in all controllers - CreateOrUpdate operation results for Kubernetes resources - Port-forwarding setup and HTTP/2 disable messages Important events like migration jobs, API sync milestones, and significant state changes remain at INFO level. Debug logging can be enabled with --v=1. Also fixes parameter schema handling in create-prefect-deployment.py by properly converting ParameterSchema objects to dictionaries using model_dump(). 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
| DeploymentLabels map[string]string `json:"deploymentLabels,omitempty"` | ||
| } | ||
|
|
||
| type PrefectServerReference struct { |
There was a problem hiding this comment.
This was hoisted up to be reusable between PrefectWorkPool and PrefectDeployment
| apiVersion: prefect.io/v1 | ||
| kind: PrefectDeployment | ||
| metadata: | ||
| name: hello-there | ||
| spec: | ||
| server: | ||
| name: prefect-ephemeral | ||
|
|
||
| workPool: | ||
| name: process-pool | ||
|
|
||
| deployment: | ||
| entrypoint: "flows/hello_world.py:hello" | ||
| pullSteps: | ||
| - prefect.deployments.steps.git_clone: | ||
| repository: https://github.com/PrefectHQ/examples.git | ||
| branch: main | ||
| parameters: | ||
| name: "prefect-operator!" | ||
| parameterOpenApiSchema: | ||
| title: Parameters | ||
| type: object | ||
| properties: | ||
| name: | ||
| default: Marvin | ||
| position: 0 | ||
| title: name | ||
| type: string | ||
| required: [] | ||
| definitions: {} |
There was a problem hiding this comment.
This is a fully working example:
Remove obvious comments that just describe what the next line does, keeping only comments that add actual value like explaining business logic or non-obvious behavior. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- Add PrefectDeploymentFinalizer constant for deployment cleanup - Modify Reconcile function to handle deletion and ensure finalizer presence - Implement two-phase reconciliation: finalizer addition then sync - Add handleDeletion method to clean up deployments from Prefect API - Fix HTTP status code handling to accept both 200 and 204 for DELETE operations - Update tests to verify finalizer behavior and deletion workflow - Fix deprecated Requeue usage in favor of RequeueAfter 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- Create convert_test.go with 100% coverage for all conversion functions - Test ConvertToDeploymentSpec with all conditional paths and error cases - Cover version info, work queue, parameters, job variables, schemas, pull steps, schedules - Test error handling for invalid JSON in all unmarshaling operations - Add edge cases for anchor date parsing and multiple schedules/pull steps - Test UpdateDeploymentStatus and GetFlowIDFromDeployment functions - Improve ConvertToDeploymentSpec coverage from 39.2% to 100% - Ensure robust validation of complex data structures and boundary conditions 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Updates controller tests to handle two-phase reconciliation pattern and finalizer lifecycle management: - Add missing import for errors.IsNotFound - Trigger reconciliation after deletion to process finalizer removal - Update error scenario tests to expect errors on second reconcile call - Fix timing issues with finalizer-based deletion in status update test All 112 tests now pass with comprehensive coverage of convert.go at 100%. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- Inlined createPrefectClient calls with direct prefect.NewClientFromK8s() usage - Inlined calculateSpecHash calls with direct utils.Hash() usage - Moved getAPIKey method from controller to PrefectServerReference.GetAPIKey() - Added comprehensive test suite for PrefectServerReference with 28 test cases - Created NewClientFromK8s function combining API key retrieval with client creation - Removed unused helper methods to reduce code indirection - Updated controller tests to use utils.Hash directly - Cleaned up imports (removed unused logr from controller) 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
| // Ensure finalizer is present | ||
| if !controllerutil.ContainsFinalizer(&deployment, PrefectDeploymentFinalizer) { | ||
| controllerutil.AddFinalizer(&deployment, PrefectDeploymentFinalizer) | ||
| if err := r.Update(ctx, &deployment); err != nil { | ||
| log.Error(err, "Failed to add finalizer", "deployment", deployment.Name) | ||
| return ctrl.Result{}, err | ||
| } | ||
| return ctrl.Result{RequeueAfter: time.Second}, nil | ||
| } |
There was a problem hiding this comment.
We're using a two-step deletion with a finalizer to allow for deleting the deployment from the Prefect server. This differs from the other controllers, because they are just managing Kubernetes objects, so there's no need for dealing with external objects.
| baseClient := NewClient("", apiKey, log) | ||
|
|
||
| // Determine if we need port-forwarding | ||
| needsPortForwarding := !baseClient.isRunningInCluster() && serverRef.IsInCluster() |
There was a problem hiding this comment.
Non-blocking: I don't fully understand the need for this yet, but I'm happy to proceed with it for now. I think I would have suspected that this controller would reach the Prefect API by either the domain on the Ingress (if external) or by the Service (if internal).
There was a problem hiding this comment.
Ahh, yes, great question, this is for local development. When developing the operator, you're running it locally on your laptop against some other cluster (like minikube, etc). Since the operator isn't actually running inside the cluster where the Prefect server is running, we can't access it with cluster-local addresses, so we need to port-forward to access it. Totally open to other approaches here, because this does add a lot of code/complexity.
There was a problem hiding this comment.
Ahh roger that. Maybe in the future we can clarify that somehow but sounds good for now!
There was a problem hiding this comment.
@chrisguidry just my two cents here but perhaps you can check out devspace or okteto. These are projects which allow you to sync source code into a running container in a given cluster including reloading/restarting the container if the local source code changes. I guess this could be used to make changes to the controller locally, have it synced and then restarted automatically in the cluster. That way you would not need the code/complexity of port forwarding. I am familiar with devspace and could help you out there, It's relatively easy to setup. Unfortunately, devspace is not actively developer anymore afaik. Hence, the okteto alternative which I have not tried yet.
There was a problem hiding this comment.
Ah thanks for the tip, I'll check that out!
4 participants
Adds the new
PrefectDeploymentCRD and a controller for it. ThisCRD will create a Prefect
Deploymentagainst the given Prefect server,which requires actually connecting to the running Prefect server for
the first time.
This also adds a Python script to the repository that helps with
generating a
PrefectDeploymentresource from a given flow, analogousto the
prefect deploy ...command. I'm aiming here to reuse as muchfrom
PrefectHQ/prefectas possible there.Note: the CRD spec ends up using
camelCasenames rather thansnake_casein order to retain consistency with the other CRDs and withKubernetes convetions.
Much of this was generated by Claude (guided by me) in several stages,
and I'll be reviewing this PR as if it were a fresh contribution.
Closes #164
Co-Authored-By: Claude noreply@anthropic.com
What Claude itself has to say about the changes:
🎯 High-Level Summary: PrefectDeployment CRD Implementation
This PR introduces a major new feature: PrefectDeployment Custom Resource Definition - allowing users to manage Prefect deployments (Python flows)
declaratively through Kubernetes. While the diff is large (~7k lines), the majority is comprehensive test coverage and generated code.
📋 What This PR Delivers
- Declarative management of Prefect deployments (Python flows)
- Supports schedules, parameters, work pools, and server connections
- Full lifecycle management with proper cleanup via finalizers
- Reconciles K8s resources with Prefect API deployments
- Handles creation, updates, deletion, and drift detection
- Robust error handling and status reporting
- HTTP client for Prefect API operations (deployments, flows)
- Support for both Prefect Cloud and self-hosted servers
- Port-forwarding for local development scenarios
🧪 Quality & Testing Focus
The large line count is primarily comprehensive test coverage:
🏗️ Architecture Improvements
- Moved API key logic from controller to PrefectServerReference domain object
- Centralized Prefect API conversion logic in dedicated modules
- Cleaner client creation patterns
- Inlined trivial helper functions to reduce indirection
- More direct method calls instead of verbose wrapper functions
- Removed unnecessary abstractions
- Comprehensive error scenarios in tests
- Proper finalizer-based cleanup to prevent resource leaks
- Graceful handling of Prefect API failures
📖 Documentation & Tooling
🎯 Review Focus Areas
Everything else is either tests, generated code, or documentation.