feat: persistent deployer in func.yaml, block incompatible deployer switch, func delete removes f.Deploy values to match reality#3953
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gauron99 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
knative-prow
Bot
added
approved
91da09f to
88f644d
Compare
There was a problem hiding this comment.
Pull request overview
This PR makes deployer selection a first-class, globally configurable setting that is persisted into func.yaml, adds a safety guard to prevent redeploying an already-deployed function with an incompatible deployer (to avoid orphaning resources), and updates delete-by-project to clear deployed markers from local state after a successful undeploy.
Changes:
- Introduce canonical deployer names (
pkg/deployers) and use them across deployer implementations and config defaults. - Persist deployer into
func.yamlvia global config/application and block incompatible deployer switches on redeploy (exceptraw -> keda). - Improve raw deployer trigger-sync behavior to tolerate forbidden trigger RBAC only when no subscriptions are declared; update docs/tests and clear deploy markers on successful project delete.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/knative/deployer.go | Switch knative deployer name constant to canonical deployers package. |
| pkg/keda/deployer.go | Switch keda deployer name constant to canonical deployers package. |
| pkg/k8s/deployer.go | Use canonical raw deployer name; add forbidden-tolerant trigger-sync choke point. |
| pkg/k8s/triggers_test.go | Add unit tests covering forbidden trigger-sync behavior and warnings. |
| pkg/deployers/deployers.go | New package defining canonical deployer names + default. |
| pkg/config/config.go | Add global Deployer field/default and apply/configure propagation to functions. |
| pkg/config/config_test.go | Ensure global config key listing includes deployer. |
| cmd/deploy.go | Add globally configurable --deployer flag defaulting chain + guard against incompatible redeploy deployer switches. |
| cmd/deploy_test.go | Add tests for deployer persistence, global seeding, and switch-guard behavior. |
| cmd/delete.go | After successful delete-by-project, clear deployed markers and write updated func.yaml. |
| cmd/delete_test.go | Add tests verifying deployed markers are cleared only for delete-by-project. |
| docs/reference/func_deploy.md | Update deploy help text to reflect deployer behavior/default. |
| docs/reference/func_delete.md | Document that delete-by-project updates local func.yaml on success. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
88f644d to
7430589
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3953 +/- ##
==========================================
+ Coverage 54.19% 54.31% +0.11%
==========================================
Files 203 204 +1
Lines 23858 23905 +47
==========================================
+ Hits 12931 12983 +52
+ Misses 9688 9680 -8
- Partials 1239 1242 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
gauron99
changed the title
lkingland
left a comment
lkingland
left a comment
There was a problem hiding this comment.
Nice! My only suggestion is to move most of that logic into core from the CLI pkg
| // Successful undeploy: clear live-deployment state so func.yaml | ||
| // reflects reality. Deploy-section config (image, options, etc.) is kept. | ||
| f.Deploy.Namespace = "" | ||
| f.Deploy.Deployer = "" | ||
| if err = f.Write(); err != nil { | ||
| return fmt.Errorf("function was undeployed from the cluster, but updating the local configuration failed: %w", err) | ||
| } | ||
| return nil |
There was a problem hiding this comment.
This might actually be part of the implementation of client.Remove
There was a problem hiding this comment.
ah thanks this is good feedback! for this to be in client some further code work was required first... Im addressing after rebase
| // path-based delete clears both Deploy.Namespace and Deploy.Deployer - | ||
| // live-deployment state that's stale once undeployed. The next flagless | ||
| // deploy correctly falls back to the global config's deployer default. | ||
| func TestDelete_ByProjectClearsDeployedMarker(t *testing.T) { |
There was a problem hiding this comment.
Likewise, this could be a test in core, because it would be doing this clearing for every deployer (even the mock on the test)
| // redeploy with a different deployer blocked by the deployer-switch | ||
| // guard - the guard only fires on an already-deployed function, and | ||
| // undeploy should've cleared up that function & func.yaml. | ||
| func TestDelete_ByProjectThenRedeployWithDifferentDeployerNotBlocked(t *testing.T) { |
There was a problem hiding this comment.
This might also be a feature/test for core: two mock deployers, different names, ensure the guard fires correctly.
| // reads or writes the func.yaml in the current directory, even when one | ||
| // exists there - the deployed-marker clearing added for path-based delete | ||
| // is scoped to that branch only. | ||
| func TestDelete_ByNameDoesNotTouchLocalFuncYaml(t *testing.T) { |
There was a problem hiding this comment.
The cmd package should have no knowledge of the fact func.yaml even exists.
The test is implemented correctly, but the comment is misleading.
| if from != to && !safeSwitch { | ||
| return fmt.Errorf("function %q was deployed with the %q deployer; redeploying with %q would orphan the old deployer's resources on the cluster - run func delete first to remove them, then redeploy", f.Name, from, to) | ||
| } |
There was a problem hiding this comment.
This is an interesting case.
My first thought is that the if from != to check would be part of core, but the part which makes a "safeSwitch" could be a backwards-compatibility if statement here in the CLI which just sets f.Deploy.Deployer to knative if it is deployed but no value was recorded.
So, essentially splitting this whole block into two pieces 1) the assertion in core and 2) the back-compat default in the cli (where implementation names are expected).
There was a problem hiding this comment.
The "safe switch check" needs to live in the CLI because the to is a cfg.Deployer value. The only way we can check for already deployed deployer value is from cluster running service - we'd need to determine what the service is, get its annotations and check.
OR we could add f.Deployer type of "what to deploy with" and f.Deploy.Deployer "what is deployed with"...
|
to resolve the tension between unclear boundaries of func.yaml purpose in terms of "half status/ half declared intention" we introduced different means to interpret some keys. usually having one yaml key represent both is fine but sometimes its not. Sometime the current state is a dependency for some action. Knowing if a function was deployed is essential to deliver "was the deployer changed". We can determine if function was deployed by checking the |
7430589 to
02b00b7
Compare
global deployer
deployers.Xvalue instead of hardcoded valuesdeployer == ""(as it was before) we default to Knative as that was the default all alongdeployer switch
raw->kedabecause keda deployer embeds the raw deployer -- eg same resources.func delete change
func deletenow actually removes thef.Deploy.Namespaceandf.Deploy.Deployervalues so thatfunc.yamlcorrectly represents the state of the function on cluster (only when deleting by project! not name since theres no local files for that flow)