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 app.kubernetes.io/name to be set in the VerticaDB CR #394

Merged
merged 7 commits into from
May 11, 2023

Conversation

roypaulin
Copy link
Collaborator

We can now set the following label in the CR:

spec:
  labels:
    app.kubernetes.io/name: "org-name"

All k8s created object will have it. If it is not set in the CR, it will assume the default value which is vertica.

A webhook has also been added to prevent other internally used labels to be overridden.

Closes #384

@roypaulin roypaulin requested a review from spilchen May 10, 2023 13:29
Copy link
Collaborator

@spilchen spilchen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Lots of little changes, but it's all pretty straightforward.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand why you have to move the labels out of the builder package. But, could we just put it in its own package rather than here? Maybe create a package named meta and put the constants for labels and annotations in it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I think putting all labels/annotations code in its own package makes more sense.

Comment on lines 393 to 394
invalidLabels := make([]string, len(ProtectedLabels))
copy(invalidLabels, ProtectedLabels)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do need to make a copy of ProtectedLabels? Could we just iterate over that one?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes we don't need.

pkg/builder/labels_annotations.go Show resolved Hide resolved
OperatorVersion110 = "1.1.0"
OperatorVersion120 = "1.2.0"
OperatorVersion130 = "1.3.0"
NameLabel = "app.kubernetes.io/name"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason this wasn't move to the package with all of the label const?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because it is the only place it is used. I am going to create a new package for labels/annotations and move all const labels there.

@roypaulin roypaulin merged commit d27885f into vertica:main May 11, 2023
@roypaulin roypaulin deleted the label branch June 23, 2023 16:50
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.

Ability to change app.kubernetes.io/name label
2 participants