-
Notifications
You must be signed in to change notification settings - Fork 189
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
K8SPXC-1230 add labels to k8s objects created by operator #1565
base: main
Are you sure you want to change the base?
Conversation
…ator into K8SPXC-1230_add_labels
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@@ -8,7 +8,17 @@ import ( | |||
) | |||
|
|||
// NewPVC returns the list of PersistentVolumeClaims for the backups | |||
func NewPVC(cr *api.PerconaXtraDBClusterBackup) *corev1.PersistentVolumeClaim { | |||
func NewPVC(cr *api.PerconaXtraDBClusterBackup, cluster *api.PerconaXtraDBCluster) *corev1.PersistentVolumeClaim { | |||
// Copy from the original labels to the backup 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.
Instead of coping these, there is this function k8s.io/apimachinery/pkg/labels.Merge()
that will merge two sets of labels, maybe you can use it.
@@ -293,6 +293,12 @@ func (r *ReconcilePerconaXtraDBCluster) deletePITR(cr *api.PerconaXtraDBCluster) | |||
ObjectMeta: metav1.ObjectMeta{ | |||
Name: deployment.GetBinlogCollectorDeploymentName(cr), | |||
Namespace: cr.Namespace, | |||
Labels: map[string]string{ | |||
"app.kubernetes.io/name": "percona-xtradb-cluster", |
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.
Wdyt that we use this opportunity and refactor this a bit so we don't repeat these strings dozens of times across the repo? To have some constants and reuse them, maybe a helper func in some labels.go
file that will add these common 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.
I checked the code. I don't know. Does it really make sense to create separate utils file to store their only one label?
As I can see the code will become more implicit, more lines of the code and we don't really win something.
What do you think? I'm ok to add it in terms of coding practice, but I really do not see much motivations for it.
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.
What I meant was that instead of having for example "app.kubernetes.io/name": "percona-xtradb-cluster",
all over the place, it will be set only in one file and then just invoke a function that will set the labels.
So the idea is that we have these common labels defined in a single place and then just reuse them where they are needed.
commit: 5c0e1da |
@nmarukovich what should we do with this PR? |
CHANGE DESCRIPTION
Problem:
Short explanation of the problem.
Cause:
Short explanation of the root cause of the issue if applicable.
Solution:
Short explanation of the solution we are providing with this PR.
CHECKLIST
Jira
Needs Doc
) and QA (Needs QA
)?Tests
compare/*-oc.yml
)?Config/Logging/Testability