Skip to content
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

allow specifying additional/default labels via command line #938

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

breunigs
Copy link
Contributor

@breunigs breunigs commented Nov 28, 2023

This change adds the ability to set additional labels or provide default values for deployment commands. This also works for ejson secrets. ejson-keys as shared secret will not be labled.

The functionality was not made available on krane render due to potentially confusing behaviour around labels on secrets when using krane render … | krane deploy -f secrets.ejson -f - .

Allowing labels specified in the templates take precedence is an intentional choice. It is the more flexible approach and allows customization for edge cases like migrations and "nested" deployments.

see #682

What are you trying to accomplish with this PR?

Setting standard/well known labels like app.kubernetes.io/name on all Kubernetes objects, but especially Secrets created through the secrets.ejson handling.

How is this accomplished?

New top-level CLI parameter on krane deploy and krane global-deploy that is passed through to the resource or ejson provider, where it they get merged into the object definition.

What could go wrong?

Edge cases around nil handling from the extra_labels side, as well as the .metadata.labels definition on the k8s object/resource side. In practice this shows up as "failed to deploy" type of error, if and only if the resource is part of the deployment templates. This might happen again if new special handlers for Resources are written that are not covered by the test in test/unit/krane/kubernetes_resource/kubernetes_resource_test.rb.

Considered Alternatives

  • re-using the selector: The dual use for pruning makes it undesirable. If the selector and old k8s objects are removed in the same deployment run, the old objects will not be pruned. Expecting the user to remember this edge case is unrealistic, especially since it's unlikely to happen often.
  • adding a .metadata.labels like key to secrets.ejson (perhaps at at .kubernetes_secrets.*._labels): I didn't test this, but it's probably possible. The reason we didn't go for this was our need to set labels on all objects, not just secrets. It would imply we need either a) manual effort of adding labels to all object definitions, or b) tooling to update these in source control in ERB files, or c) some extra tool that patches the secrets.ejson before passing it to krane render … | krane deploy -f secrets.ejson -f -. So while conceptually simpler in krane, it has limited utility in organically grown systems – or at least ours. Hence it was discarded early on.

On CLA
Your CLA flow claims someone signed for my account already, although we're not sure who anymore. I also have recent permission by someone with signing power from my org. Therefore I expect this at most to require some bureaucratic cleanup, but not present a blocker.

This change adds the ability to set additional labels or provide
default values for deployment commands. This also works for ejson
secrets. `ejson-keys` as shared secret will not be labled.

The functionality was not made available on `krane render` due to
potentially confusing behaviour around labels on secrets when
using `krane render … | krane deploy -f secrets.ejson -f -` .

Allowing labels specified in the templates take precedence is an
intentional choice. It is the more flexible approach and allows
customization for edge cases like migrations and "nested"
deployments.

see Shopify#682
@breunigs
Copy link
Contributor Author

breunigs commented Nov 28, 2023

Okay, the CLA bot overwrote the actual test CI result, which is only partially successful. As far as I can tell, the cases that fail on my PR also fail on main, and are unrelated to my changed.

@breunigs breunigs marked this pull request as ready for review November 28, 2023 16:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant