-
Notifications
You must be signed in to change notification settings - Fork 3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: local apply #142
feat: local apply #142
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## dev #142 +/- ##
==========================================
- Coverage 54.33% 52.72% -1.61%
==========================================
Files 23 24 +1
Lines 2306 2420 +114
==========================================
+ Hits 1253 1276 +23
- Misses 870 960 +90
- Partials 183 184 +1 ☔ View full report in Codecov by Sentry. |
@fritzduchardt does it make sense to apply all envs to the current kubernetes context? |
@kbudde Isn't that what you did for your home automation ;). I am using local apply for both my clients at the moment. In one case I have not set up Flux yet. In the other, it is useful to try changes out before commit, e.g. when setting up new apps. However, I could think of a couple of ways to validate that you are on the right cluster. Let's talk about it in the next community meeting. |
d142de9
to
b8fc55a
Compare
I'd still like to clarify the reasoning behind this proposal a little more. In the corresponding issue, I was asking:
|
var helmNamespace, argoNamespace, namespace string | ||
helmNamespace = a.HelmConfig.Namespace | ||
if a.ArgoConfig != nil { | ||
argoNamespace = a.ArgoConfig.App.Destination.Namespace | ||
} | ||
namespace, err := decideNamespace(helmNamespace, argoNamespace, a.Name) | ||
if err != nil { | ||
return err | ||
} | ||
// check namespace exists, if not create it | ||
_, err = a.runCmd(applyStepName, "check namespace exists", "kubectl", nil, []string{"get", "namespace", namespace}) | ||
if err != nil { | ||
_, err = a.runCmd(applyStepName, "create namespace", "kubectl", nil, []string{"create", "namespace", namespace}) | ||
log.Info().Msg("Create namespace: " + namespace) | ||
if err != nil { | ||
return err | ||
} | ||
} | ||
// set default namespace | ||
configArgs := []string{"config", "set-context", "--current", "--namespace", namespace} | ||
_, err = a.runCmd(applyStepName, "set default namespace", "kubectl", nil, configArgs) | ||
if err != nil { | ||
return err | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Following the "source of truth" and "explicit better than implicit" ideas, I'd rather force users to explicitly define namespaces in their resources.
We can provide an overlay with the logic similar to the logic currently implemented in this code. Something similar to this:
#! This file contains YTT overlay for setting namespace to resources without namespace.
#@ load("@ytt:data", "data")
#@ load("@ytt:overlay", "overlay")
#@ def select_for_namespace(i, l, r):
#@ if "metadata" in l:
#@ return "namespace" not in l["metadata"] or not l["metadata"]["namespace"]
#@ end
#@ return False
#@ end
#@ d = data.values
#@ namespace = d.argocd.app.destination.namespace or d.helm.namespace or d.myks.context.app
#@overlay/match by=select_for_namespace, when="1+"
---
#@overlay/match-child-defaults missing_ok=True
metadata:
namespace: #@ namespace
We should consider providing an explicit way to set the namespace for the application. Before it was not needed, because we were setting the namespace via the ArgoCD part of the configuration. Now, when we make ArgoCD optional, we should have another way. In the past, I remember using just .application.namespace
when needed. This can be sufficient, but I'm not entirely sure if we should invade this "user-defined" area of configuration with pre-defined options.
Another minor point is about the usage of kubectl
. Instead of changing the current namespace, one can simply use --namespace
flag of kubectl
.
Superseded by the awesome new plugin feature |
/closes #138