-
Notifications
You must be signed in to change notification settings - Fork 115
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
Add option select-any for deployTask and globalDeployTask #831
Conversation
As discussed in Slack, perhaps a more appropriate name for this flag is If this indeed is the most straightforward solution then I won't block it, but I strongly urge you to consider other ways in which we can accomplish this goal (e.g. filtering the resources before passing them to Again, I won't block if it's decided that this is indeed the best way forward, but it's not my first choice. cc @karanthukral interested to know your thoughts as one of the other krane maintainers |
After some discussion on slack, I understand the concerns around adding this as a feature to Krane but at the same time think its a valid trade off. I agree with the more explicit naming approach and setting the new feature to The deploy tooling internally is built to not care about what it's actually deploying and we don't have an appropriate area to add the extra logic prior to templates being passed to Krane. Due to this I think despite this not being the most ideal approach, it's a valid tradeoff given the requirements and constraints the team is under. |
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.
A few small nits, overall the code looks fine. We should update the README to be a bit more explicit about this feature, since it's a bit of a special case whose use isn't immediately clear. In particular, we should be crystal clear about its intent (see readme suggestions). We should also explain that it preserves the pruning behaviour of selector
as well.
README.md
Outdated
@@ -117,6 +117,7 @@ Refer to `krane help` for the authoritative set of options. | |||
- `--global-timeout=duration`: Raise a timeout error if it takes longer than _duration_ for any | |||
resource to deploy. | |||
- `--selector`: Instructs krane to only prune resources which match the specified label selector, such as `environment=staging`. If you use this option, all resource templates must specify matching labels. See [Sharing a namespace](#sharing-a-namespace) below. | |||
- `--selector-as-filter`: Instructs krane to only deploy resources that are filtered by the specified labels in `--selector`. The deploy will not fail if not all resources match the labels. |
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.
nit/suggestion:
- `--selector-as-filter`: Instructs krane to only deploy resources that are filtered by the specified labels in `--selector`. The deploy will not fail if not all resources match the labels. | |
- `--selector-as-filter`: Instructs krane to only deploy resources that are filtered by the specified labels in `--selector`. The deploy will not fail if not all resources match the labels. This is useful if you only want to deploy a subset of resources within a given YAML file. |
README.md
Outdated
@@ -441,6 +442,7 @@ Refer to `krane global-deploy help` for the authoritative set of options. | |||
- `--filenames / -f [PATHS]`: Accepts a list of directories and/or filenames to specify the set of directories/files that will be deployed. Use `-` to specify STDIN. | |||
- `--no-prune`: Skips pruning of resources that are no longer in your Kubernetes template set. Not recommended, as it allows your namespace to accumulate cruft that is not reflected in your deploy directory. | |||
- `--selector`: Instructs krane to only prune resources which match the specified label selector, such as `environment=staging`. By using this option, all resource templates must specify matching labels. See [Sharing a namespace](#sharing-a-namespace) below. | |||
- `--selector-as-filter`: Instructs krane to only deploy resources that are filtered by the specified labels in `--selector`. The deploy will not fail if not all resources match the labels. |
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.
nit/suggestion:
- `--selector-as-filter`: Instructs krane to only deploy resources that are filtered by the specified labels in `--selector`. The deploy will not fail if not all resources match the labels. | |
- `--selector-as-filter`: Instructs krane to only deploy resources that are filtered by the specified labels in `--selector`. The deploy will not fail if not all resources match the labels. This is useful if you only want to deploy a subset of resources within a given YAML file. |
lib/krane/cli/deploy_command.rb
Outdated
@@ -25,6 +25,9 @@ class DeployCommand | |||
default: true }, | |||
"selector" => { type: :string, banner: "'label=value'", | |||
desc: "Select workloads by selector(s)" }, | |||
"selector-as-filter" => { type: :boolean, | |||
desc: "Use --selector as a label filter to select a subset of resources to deploy", |
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.
desc: "Use --selector as a label filter to select a subset of resources to deploy", | |
desc: "Use --selector as a label filter to deploy only a subset of the provided resources to deploy", |
@@ -16,6 +16,9 @@ class GlobalDeployCommand | |||
desc: "Verify workloads correctly deployed" }, | |||
"selector" => { type: :string, banner: "'label=value'", required: true, | |||
desc: "Select workloads owned by selector(s)" }, | |||
"selector-as-filter" => { type: :boolean, | |||
desc: "Use --selector as a label filter to select a subset of resources to deploy", |
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.
desc: "Use --selector as a label filter to select a subset of resources to deploy", | |
desc: "Use --selector as a label filter to deploy only a subset of the provided resources to deploy", |
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.
A few small nits, overall the code looks fine. We should update the README to be a bit more explicit about this feature, since it's a bit of a special case whose use isn't immediately clear. In particular, we should be crystal clear about its intent (see readme suggestions). We should also explain that it preserves the pruning behaviour of
selector
as well.
Added some explanation in README Sharing Namespace
.
@@ -166,6 +166,40 @@ def test_selector | |||
assert_equal("master", deployments.first.metadata.labels.branch) | |||
end | |||
|
|||
def test_select_any | |||
# Deploy only the resource matching the selector without valiation error |
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.
nit: valiation -> validation
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.
Thanks for updating the docs. I have a few more minor comments but nothing too major.
lib/krane/version.rb
Outdated
@@ -1,4 +1,4 @@ | |||
# frozen_string_literal: true | |||
module Krane | |||
VERSION = "2.1.10" | |||
VERSION = "2.1.11" |
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.
Usually I make a dedicated branch when bumping the version, so I can tie up any loose ends that are around just in case. For now, let's put this back to 2.1.10 and I can cut a release when this merges
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.
ok, I reverted the version here. Not sure if you want me to keep the update in CHANGELOG.md.
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.
The CHANGELOG entry is fine (and appreciated!)
lib/krane/global_deploy_task.rb
Outdated
@@ -143,6 +146,8 @@ def validate_resources(resources) | |||
end | |||
raise FatalDeploymentError, "Template validation failed" | |||
end | |||
|
|||
resources.select! { |r| r.selected?(@selector) } |
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.
Any reason we can't move this up before we call validate_definition
on the resources? That way validate_definition
doesn't need to worry about whether or not selector_as_filter
is set.
e.g.
resources.select! { |r| r.selected?(@selector) } if @selector_as_filter
Concurrency.split_across_threads(resources) do |r|
r.validate_definition(kubectl: @kubectl, selector: @selector)
end
This would be nicer because otherwise we are leaking the filtering concern into kubernetes_resource
.
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.
One last nit, but I think this is good
lib/krane/deploy_task.rb
Outdated
@@ -273,6 +275,7 @@ def validate_configuration(prune:) | |||
|
|||
confirm_ejson_keys_not_prunable if prune | |||
@logger.info("Using resource selector #{@selector}") if @selector | |||
@logger.info("Can deploy a subset of resources") if @selector && @selector_as_filter |
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.
Perhaps something a bit more explicit:
@logger.info("Can deploy a subset of resources") if @selector && @selector_as_filter | |
@logger.info("Only deploying resources that match filter: #{@selector_as_filter}") if @selector && @selector_as_filter |
Related PR: https://github.com/Shopify/gkrane-config/pull/45
What are you trying to accomplish with this PR?
Currently option
--selector
for sub commandkrane deploy
andkrane global-deploy
will cause validation failure if any k8s resources in templates (--filenames) does not match the label selectors. This will cause problems when we want to deploy a subset of k8s resources.How is this accomplished?
Adding a new option
selector-as-filter
in boolean withdefault
value of false. If set totrue
, selector validation will be skipped. Only resources with matching labels will be deployed and be prunable.This allows us to deploy none or a subset of resources. It will help us to address canary and production deploy where the 2 sets of resources share the same namespace and same templates file directory.
What could go wrong?
The new option has default value of
false
, which will not change current selector behaviour. Therefore it should be backward compatible. This is not a breaking change.