Skip to content
This repository has been archived by the owner on Jun 4, 2024. It is now read-only.

feat: update pods deployment labels #973

Closed
wants to merge 2 commits into from

Conversation

thib-mary
Copy link
Contributor

@thib-mary thib-mary commented Nov 27, 2023

Describe your changes

Update labels in the pod template spec of the deployment to include the full set of labels and not just the selector labels.

@thib-mary thib-mary force-pushed the patch-1 branch 2 times, most recently from 1080518 to f8ce5a9 Compare December 4, 2023 09:26
@thib-mary thib-mary force-pushed the patch-1 branch 2 times, most recently from 1bd686d to aefb472 Compare December 7, 2023 08:37
@thib-mary
Copy link
Contributor Author

Hi klizhentas, russjones, r0mant, Joerger, tcsc, zmb3, camscale and fheinecke, could i have a review please ? 🙏

@camscale
Copy link
Contributor

camscale commented Dec 10, 2023

@thib-mary Would you describe your changes a little more please? Describe the change as you see it and why this should be done. Something like:

Change the labels in the pod template spec of the deployment in the slack chart to include the full
set of labels and not just the selector labels. This includes the help chart, app version and managed-by labels.
These are needed because ...

The reason I ask is because it seems wrong to set the managed-by label to helm as helm does not manage the pod. Some of the other labels may make sense.

However if this change does make sense, it needs to be made to all the helm charts and not just for slack. I've asked for the team's input into why the labels might have been limited to just the selector labels. I'll update this when I find out more.

@camscale
Copy link
Contributor

@thib-mary I've verified with the team that this change is correct and is how the helm chart should have been written. Would you like to update all the helm charts to fix this across the repo, or would you prefer I do that? Happy either way.

I've approved the workflow runs so you can see that there are tests that are failing now because of this change. But looking at them now, it seems there are some other tests failing perhaps due to unrelated changes that I will need to look into.

@thib-mary
Copy link
Contributor Author

@thib-mary I've verified with the team that this change is correct and is how the helm chart should have been written. Would you like to update all the helm charts to fix this across the repo, or would you prefer I do that? Happy either way.

I've approved the workflow runs so you can see that there are tests that are failing now because of this change. But looking at them now, it seems there are some other tests failing perhaps due to unrelated changes that I will need to look into.

Thanks for the review. I have updated the pull request to update all the concerned helm charts.

thib-mary and others added 2 commits January 31, 2024 11:02
## Describe your changes

Update labels on pods managed by deployment.
@tigrato
Copy link
Contributor

tigrato commented Jan 31, 2024

Hi @thib-mary,

Thanks for your contribution. We are currently unable to run our tests on
external contributions. As such, I'll be helping you validate and
merge your pull request. I've created an internal PR here:

#1013

Please wait for another approver and I'll get your
code merged. You will retain authorship of your commits.

@tigrato tigrato closed this Jan 31, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants