Fix no namespace resource found on delete resource k8s #1281
Conversation
| // getNamespaceForRun returns namespace used on kubectl apply/delete commands | ||
| // priority: config.KubernetesDeploymentInput > kubernetes.ResourceKey | ||
| func (p *provider) getNamespaceForRun(k ResourceKey) string { | ||
| if p.input.Namespace != "" { | ||
| return p.input.Namespace | ||
| } | ||
| return k.Namespace |
There was a problem hiding this comment.
We could get the namespace from ResourceKey ( from manifest files ) or from KubernetesDeploymentInput ( which could be empty on unset currently ). IMO, namespace value from KubernetesDeploymentInput should have a higher priority than the value from ResourceKey, in case of mismatch, the current not found resources to delete error will be raised.
How do you think?
There was a problem hiding this comment.
I think the one in the user-defined manifest should be more prioritized than pipe.yaml when applying.
There was a problem hiding this comment.
To be simple, the piped's role is basically just to perform kubectl apply -f manifest.yaml.
There was a problem hiding this comment.
Hmm... but on the second thought, the case that user clearly defines its namespace in the .pipe.yaml, I'm begging to feel it's better to override the base manifest.
There was a problem hiding this comment.
One vote from me for taking the namespace value from the Input with higher priority.
There was a problem hiding this comment.
Hmm... but on the second thought, the case that user clearly defines its namespace in the
.pipe.yaml, I'm begging to feel it's better to override the base manifest.
yep, I feel the same either. Since the manifest should be treated as a base, and config on .pipe.yaml lay on that base and those who defined the config should be aware of where should the deployment they set will be applied to.
|
/cc @nakabonne |
|
Code coverage for golang is
|
|
Code coverage for golang is
|
|
Nice catch! |
| return p.kubectl.Delete(ctx, p.getNamespaceToRun(k), k) | ||
| } | ||
|
|
||
| // getNamespaceToRun returns namespace used on kubectl apply/delete commands |
There was a problem hiding this comment.
nit nit: . at end of the line. (Just the convention of our team. )
|
Except for the stuff mentioned by nghialv, looks good. Great work 👍 |
|
Code coverage for golang is
|
|
Great. Thank you. |
|
This pull request is APPROVED by Approvers can cancel the approval by writing |
What this PR does / why we need it:
The
namespacein KubernetesDeploymentInput config currently is not a required field. In case of deleting resource on unset that namespace field, namespace default will be used ( as a default namespace of k8s ) leads tonot found resources to deletebug.Which issue(s) this PR fixes:
Fixes #1275
Does this PR introduce a user-facing change?: