Override app namespace #259
Conversation
permits overriding an app's namespace enable app.spec.namespace on delete
stinkyfingers
force-pushed
the
app-namespace
branch
from
a4d41e7
to
4c86644
Compare
| @@ -46,7 +46,7 @@ kind: Deployment | |||
| metadata: | |||
| annotations: | |||
| app: app1 | |||
| theketch.io/is-shipa: "true" | |||
| shipa.io/is-shipa: "true" | |||
There was a problem hiding this comment.
should we keep theketch.io ?
There was a problem hiding this comment.
This was changed to theketch.io so a testcase would pass, makes sense to me to keep as theketch.io https://github.com/theketchio/ketch/blob/main/internal/chart/testdata/render_yamls/annotations-patch.yaml#L5
There was a problem hiding this comment.
Looks like a rebase got this caught up.
| func (r *AppReconciler) UpdateAppNamespaceLabelsForIstio(ctx context.Context, app *ketchv1.App) error { | ||
| var framework ketchv1.Framework | ||
| err := r.Client.Get(ctx, types.NamespacedName{Name: app.Spec.Framework, Namespace: app.Namespace}, &framework) | ||
| fmt.Println("E", err) |
There was a problem hiding this comment.
let's remove this Println?
| if err != nil { | ||
| return err | ||
| } | ||
| namespace.Labels["istio-injection"] = "enabled" |
There was a problem hiding this comment.
should we remove "istio-injection" label for other supported ingresses?
There was a problem hiding this comment.
Modified this function to remove this istio-injection label from the namespace when not istio. Also we now reconcile the sidecar.istio.io/inject label on Pods.
| namespace.Labels["istio-injection"] = "enabled" | ||
| if framework.Spec.IngressController.IngressType != ketchv1.IstioIngressControllerType { | ||
| delete(namespace.Labels, "istio-injection") | ||
| app.AddLabel(map[string]string{"sidecar.istio.io/inject": "false"}, ketchv1.Target{Kind: "Pod", APIVersion: "v1"}) |
There was a problem hiding this comment.
with "sidecar.istio.io/inject": "false" label should we just keep a namespace as it is?
meaning we really don't need to care about a namespace's labels.
There was a problem hiding this comment.
Sounds good. I removed all the namespace label changes.
internal/api/v1beta1/app_types.go
Outdated
| if app.Spec.Labels[i].Target != target { | ||
| continue | ||
| } | ||
| for key := range app.Spec.Labels[i].Apply { |
There was a problem hiding this comment.
that's a very tricky function
should it be
for key := range labels {
delete(app.Spec.Labels[i].Apply, key)
}?
There was a problem hiding this comment.
Good catch. Changed.
…app.spec.namespace bug; add namespace override for job
|
I opted to update this to include a |
Description
Permits overriding an App's namespace. Apps default to the namespace specified by
framework.spec.namespace, but there are cases where we don't want namespace tied to framework.Fixes # (issue)
Type of change
Testing
Documentation
Final Checklist:
Additional Information (omit if empty)
Please include anything else relevant to this PR that may be useful to know.