-
Notifications
You must be signed in to change notification settings - Fork 65
Correctly remove the dynamic replicaset from the pod_name #100
Correctly remove the dynamic replicaset from the pod_name #100
Conversation
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.
Hey, this looks great! Added some very minor comments but otherwise the k8s-reverse-engineering logic looks perfect.
|
||
def to_hash(pod_template_hash) | ||
# Convert the pod_template_hash to an alphanumeric string using the same | ||
# logic Kubernetes uses at https://github.com/kubernetes/apimachinery/blob/master/pkg/util/rand/rand.go#L119 |
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.
You should use a permanent link here in case this module moves:
@@ -28,6 +28,31 @@ def is_number?(string) | |||
true if Float(string) rescue false | |||
end | |||
|
|||
def strip_dynamic_bits(k8s_metadata) |
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.
I'd recommend def sanitize_pod_name
Thanks for the suggestions, I've updated the PR with the requested changes. |
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.
Awesome!
Hey @frankreno, this implements the suggestion I made in #78 (comment). Should be good to merge when you have a chance! |
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.
LGTM
@bendrucker thanks for reviewing! @nmohoric - Thanks for the PR! I am going to merge, but want to give a couple of more days for the pending PRs I just reviewed as well as submit a PR to upgrade the FluentD Output Plugin. I will add a comment when I cut a new release with the changes, next week sometime. |
v2.1.0 is out with this change |
This should address #78