-
Notifications
You must be signed in to change notification settings - Fork 116
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
Switch namespaced deploy to pruning blacklist #616
Conversation
Namespaced pruneable resources in k8s v1.16.1"/v1/ConfigMap", Newly prunable resources"Endpoints", "Event", "LimitRange", "ReplicationController", "ControllerRevision", "Lease", "Event" |
86a79f2
to
8bf43b6
Compare
8aeb382
to
d91d9ef
Compare
@@ -8,5 +8,9 @@ def initialize(**args) | |||
raise "Use Krane::DeployGlobalTask to deploy global resources" if args[:allow_globals] | |||
super(args.merge(allow_globals: false)) | |||
end | |||
|
|||
def prune_whitelist |
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.
are we adding this here (instead of updating Krane::DeprecatedDeployTask
) to avoid changing KubernetesDeploy::DeployTask
?
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.
Yes
This PR and the global-deploy pruning PR no longer respect the |
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 questions. In general I'm finding this pretty hard to read and understand, though :(
pattern = /v(?<major>\d+)(beta)?(?<minor>\d+)?/ | ||
latest = versions.sort_by do |version| | ||
match = version.match(pattern) | ||
[match[:major].to_i, (match[:minor] || 999).to_i] | ||
end.last | ||
version_override.fetch(kind, latest) | ||
end |
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.
Could you explain a bit further on exactly what's happening here? Do we only need to worry about (beta)
, what about alpha
groups, or are they not relevant (e.g. we only care about the override group)?
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.
Good point, I've also tried to fix up names and added a comment to make things clearer.
%w(PriorityClass StorageClass).each do |expected_kind| | ||
assert kinds.one? { |k| k.include?(expected_kind) } |
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.
Is this just a sanity check or is there a reason for these 2 in particular?
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.
Just a sanity check, the assert_equal(kinds.length, 13)
is the check to ensure list has the right number of items.
@@ -185,8 +185,7 @@ def namespace_globals(fixtures, selector) | |||
fixtures.each do |_, kinds_map| | |||
kinds_map.each do |_, resources| | |||
resources.each do |resource| | |||
resource["metadata"]["name"] = (resource["metadata"]["name"] + @namespace)[0..63] | |||
resource["metadata"]["name"] += "0" if resource["metadata"]["name"].end_with?("-") | |||
resource["metadata"]["name"] = (resource["metadata"]["name"] + Digest::MD5.hexdigest(@namespace))[0..63] |
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.
This is to fix a bug that was causing resources to have the same name across different tests. Its triggered when the real name was long as the start of two tests were the same. We need the value appended to the name to be stable across tests hence the hashing of @namespace.
d91d9ef
to
1460d6a
Compare
What are you trying to accomplish with this PR?
Prune all namespaced kinds by default instead of maintaining a whitelist.
How is this accomplished?
This mirrors #612. However, the latest group/versions doesn't always include all kinds, so I've added an override for the 5 kinds that need it. As far as I can tell there is no kubectl call to get all resources supported version. There is a call to get a resource's supported version making a call per resource seems extreme.
What could go wrong?
This will prune things we didn't before, full list is below..
We may have to keep the override list up-to-date. That being said the list currently contains five items, which is much shorter than the white list it's replacing.