-
Notifications
You must be signed in to change notification settings - Fork 17
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 extra labels with smbios data #155
Conversation
Now that we get the smbios data properly, we can add extra labels to the inventory based on that smbios data. This could be helpful to identify groups of machines in big deployments and adds extra info to the inventory so we can search using selectors via kubectl. Signed-off-by: Itxaka <[email protected]>
final labels ends like this:
|
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.
Looks good to me 👍 Registering machine details looks like a good move to me, I agree this is likely to be useful in large deployments.
I just do not understand the naming start constraints and its change.
@@ -43,7 +43,7 @@ const defaultName = "m-${System Information/Manufacturer}-${System Information/P | |||
var ( | |||
sanitize = regexp.MustCompile("[^0-9a-zA-Z]") | |||
doubleDash = regexp.MustCompile("--+") | |||
start = regexp.MustCompile("^[a-zA-Z]") | |||
start = regexp.MustCompile("^[a-zA-Z0-9]") |
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.
Why is required? I don't know from where this restriction came from, but I am wondering if relaxing it will cause issues.
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 think it was used just to check if the label (was the name) had a valid first character, otherwise m
is prepended.
Yep, this code is a bit cumbersome maybe it deserves a rework 😅
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.
Then as a result of this change a name staring with a digit has no m
prepended right? will it be an issue?
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.
AFAIK no, kubernetes resource names and labels may start with a digit too
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.
yep, this was done only with the machine name in mind, so the regex never expected to find a machine name starting with a digit :D
This is mostly due to protection against someone setting a machine name like -sdfa
or an empty name ""
And yeah k8s names/labels are alphanumeric + -
and .
but not at the start or end -> https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names
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.
Really geat idea, love this patch! ❤️
Now that we get the smbios data properly, we can add extra labels to the
inventory based on that smbios data.
This could be helpful to identify groups of machines in big deployments
and adds extra info to the inventory so we can search using selectors
via kubectl.
Signed-off-by: Itxaka [email protected]