-
Notifications
You must be signed in to change notification settings - Fork 30
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
chore: Make managed-by label value transparent #1894
base: main
Are you sure you want to change the base?
Conversation
f6ab94a
to
00fb14a
Compare
dfc5c6a
to
243eaaf
Compare
5819222
to
243eaaf
Compare
243eaaf
to
ec2bd89
Compare
deb330d
to
ec2bd89
Compare
c45c1c2
to
ec2bd89
Compare
d1dc534
to
9820cb9
Compare
9820cb9
to
9c71592
Compare
483e284
to
c0f5beb
Compare
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 found the following resources that seem to missing the label:
apiVersion: v1
kind: Service
metadata:
creationTimestamp: "2024-09-30T11:21:49Z"
name: skr-webhook-metrics
namespace: kyma-system
resourceVersion: "594"
uid: 5d8db776-d102-404b-a46e-6fe17bf22f6f
spec:
clusterIP: 10.43.161.169
clusterIPs:
- 10.43.161.169
internalTrafficPolicy: Cluster
ipFamilies:
- IPv4
ipFamilyPolicy: SingleStack
ports:
- name: http-metrics
port: 2112
protocol: TCP
targetPort: metrics-port
selector:
app: skr-webhook
sessionAffinity: None
type: ClusterIP
status:
loadBalancer: {}
---
apiVersion: v1
kind: Service
metadata:
creationTimestamp: "2024-09-30T11:21:49Z"
name: skr-webhook-metrics
namespace: kyma-system
resourceVersion: "594"
uid: 5d8db776-d102-404b-a46e-6fe17bf22f6f
spec:
clusterIP: 10.43.161.169
clusterIPs:
- 10.43.161.169
internalTrafficPolicy: Cluster
ipFamilies:
- IPv4
ipFamilyPolicy: SingleStack
ports:
- name: http-metrics
port: 2112
protocol: TCP
targetPort: metrics-port
selector:
app: skr-webhook
sessionAffinity: None
type: ClusterIP
status:
loadBalancer: {}
---
apiVersion: v1
items:
- apiVersion: operator.kyma-project.io/v1alpha1
kind: Sample
metadata:
creationTimestamp: "2024-09-30T11:26:00Z"
finalizers:
- sample.kyma-project.io/finalizer
generation: 1
name: sample-yaml
namespace: kyma-system
resourceVersion: "783"
uid: d90013b9-e90c-4205-85dc-23122a1be6c6
spec:
resourceFilePath: ./module-data/yaml
status:
conditions:
- lastTransitionTime: "2024-09-30T11:26:01Z"
message: installation is ready and resources can be used
observedGeneration: 1
reason: Ready
status: "True"
type: Installation
state: Ready
kind: List
metadata:
resourceVersion: ""
---
apiVersion: v1
kind: ServiceAccount
metadata:
creationTimestamp: "2024-09-30T11:25:58Z"
name: default
namespace: template-operator-system
resourceVersion: "735"
uid: 35585fc0-924a-4f37-85d2-ffc5a7cc288f
---
Do we also have documentation on the label that may need to be changed?
internal/remote/crd_upgrade.go
Outdated
labels := crdToApply.GetLabels() | ||
if labels == nil { | ||
labels = map[string]string{} | ||
} | ||
labels[shared.ManagedBy] = shared.KymaLabelValue | ||
crdToApply.SetLabels(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.
It bugs me a bit that Go is so verbose here... Not a problem with your implementation but the language... Should we at least introduce a helper func that merges two sets of labels so that we don't have to the same logic 3 times?
Something like
func mergeMaps(map1, map2 map[string]string) map[string]string {
mergedMap := make(map[string]string)
for k, v := range map1 {
mergedMap[k] = v
}
for k, v := range map2 {
mergedMap[k] = v
}
return mergedMap
}
// usage
crdToApply.SetLabels(mergeMaps(crdToApply.GetLabels(), map[string]string{
shared.ManagedBy: shared.KymaLabelValue,
}))
I also played around with maps.Copy
std func but that one fails if the destination map is nil which could be the case for us. Ideally, we could also make the mergeMaps func generic instead of string string.
WDYT?
internal/remote/remote_catalog.go
Outdated
labels := moduleTemplate.GetLabels() | ||
if labels == nil { | ||
labels = map[string]string{} | ||
} | ||
labels[shared.ManagedBy] = shared.KymaLabelValue | ||
moduleTemplate.SetLabels(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.
+1 on the map topic
pkg/watcher/skr_webhook_resources.go
Outdated
labels := deployment.GetLabels() | ||
if labels == nil { | ||
labels = make(map[string]string) | ||
} | ||
labels[shared.ManagedBy] = shared.KymaLabelValue | ||
deployment.SetLabels(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.
+1 on the map topic
lbls := resource.GetLabels() | ||
if lbls == nil { | ||
lbls = make(map[string]string) | ||
} | ||
lbls[shared.WatchedByLabel] = shared.WatchedByLabelValue | ||
lbls[shared.ManagedBy] = shared.ManagedByLabelValue |
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 noticed this would be another +1 for the merge maps
Description
Changes proposed in this pull request:
Related issue(s)
Resolves #1821