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

Helm: Add service labels and annotations #10214

Merged

Conversation

rubenvw-ngdata
Copy link
Contributor

What this PR does / why we need it:
Support setting labels and annotations for all services in the Helm chart.
Move label configuration for read, write and gateway services to #component.service.labels to improve
consistency between services.

Which issue(s) this PR fixes:
Fixes #8627

Special notes for your reviewer:

Checklist

  • Reviewed the CONTRIBUTING.md guide (required)
  • CHANGELOG.md updated
  • For Helm chart changes bump the Helm chart version in production/helm/loki/Chart.yaml and update production/helm/loki/CHANGELOG.md and production/helm/loki/README.md. Example PR

@rubenvw-ngdata rubenvw-ngdata requested a review from a team as a code owner August 10, 2023 13:54
@CLAassistant
Copy link

CLAassistant commented Aug 10, 2023

CLA assistant check
All committers have signed the CLA.

@rubenvw-ngdata rubenvw-ngdata force-pushed the #8627_helm_service_annotations branch 2 times, most recently from d3e4c3d to b98f265 Compare August 10, 2023 14:18
@github-actions github-actions bot added the type/docs Issues related to technical documentation; the Docs Squad uses this label across many repositories label Aug 10, 2023
@rubenvw-ngdata rubenvw-ngdata force-pushed the #8627_helm_service_annotations branch 5 times, most recently from 427d3b1 to d84668a Compare August 11, 2023 09:48
@rubenvw-ngdata
Copy link
Contributor Author

Is this going to be reviewed soon? We are waiting for this functionality.

Copy link
Contributor

@MasslessParticle MasslessParticle left a comment

Choose a reason for hiding this comment

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

LGTM, Thanks for contributing!

@rubenvw-ngdata rubenvw-ngdata force-pushed the #8627_helm_service_annotations branch 2 times, most recently from c0977ce to bc2759d Compare August 31, 2023 15:26
@MichelHollands
Copy link
Contributor

@rubenvw-ngdata Thanks for this contribution. Can you fix the issues raised by CI?

@MichelHollands
Copy link
Contributor

@rubenvw-ngdata Also, can you use version 5.17? That would help with some another PR waiting to be merged.

@MichelHollands
Copy link
Contributor

@rubenvw-ngdata I updated this but have encountered a problem. I'll come back to this tomorrow.

@rubenvw-ngdata
Copy link
Contributor Author

@MichelHollands Any update on this? Anything I should still do?

@MichelHollands
Copy link
Contributor

@MichelHollands Any update on this? Anything I should still do?

@rubenvw-ngdata Sorry, I'm busy with some other things. Can you check why the CI run fails? If that is fixed it can be merged.

Copy link
Collaborator

@trevorwhitney trevorwhitney left a comment

Choose a reason for hiding this comment

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

The templates are currently failing to render, at least with ci/default-values.yaml. Are you able to run a helm template locally using those values? I've tried and it fails, seems to be a problem with the format of the yaml templates as I keep getting the error converting YAML to JSON error.

@rubenvw-ngdata
Copy link
Contributor Author

@trevorlauder I think the helm template issue is an issue that was recently introduced on main.
I'm getting errors now too, but I'm pretty certain that I have tested this and there were no issues.
Getting the same issue when I run it on the current main branch.

@MichelHollands
Copy link
Contributor

MichelHollands commented Sep 6, 2023

@rubenvw-ngdata You are right: it was a problem on the main branch. It is failing for a different reason now.

@rubenvw-ngdata rubenvw-ngdata force-pushed the #8627_helm_service_annotations branch from 862a65f to 0366ead Compare September 7, 2023 08:32
Support setting labels and annotations for all services
in the Helm chart.
Move label configuration for read, write and table
manager services to #component.service.labels to improve
consistency between components.
@rubenvw-ngdata rubenvw-ngdata force-pushed the #8627_helm_service_annotations branch from 0366ead to 9bc1c84 Compare September 7, 2023 08:52
@rubenvw-ngdata
Copy link
Contributor Author

@trevorwhitney @MichelHollands I think everything is ok now. Let me know if I should do anything else.

Copy link
Contributor

@MichelHollands MichelHollands left a comment

Choose a reason for hiding this comment

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

LGTM

@MichelHollands MichelHollands dismissed trevorwhitney’s stale review September 7, 2023 09:38

The requested changes were applied. Dismissing so I can merge.

@MichelHollands MichelHollands merged commit f0762b3 into grafana:main Sep 7, 2023
5 checks passed
@MichelHollands
Copy link
Contributor

@trevorwhitney @MichelHollands I think everything is ok now. Let me know if I should do anything else.

@rubenvw-ngdata Thanks for your help and sorry for the hold-up. It is merged now.

rhnasc pushed a commit to inloco/loki that referenced this pull request Apr 12, 2024
**What this PR does / why we need it**:
Support setting labels and annotations for all services in the Helm
chart.
Move label configuration for read, write and gateway services to
#component.service.labels to improve
consistency between services.

**Which issue(s) this PR fixes**:
Fixes grafana#8627 

**Special notes for your reviewer**:

**Checklist**
- [x] Reviewed the
[`CONTRIBUTING.md`](https://github.com/grafana/loki/blob/main/CONTRIBUTING.md)
guide (**required**)
- [x] `CHANGELOG.md` updated
- [x] For Helm chart changes bump the Helm chart version in
`production/helm/loki/Chart.yaml` and update
`production/helm/loki/CHANGELOG.md` and
`production/helm/loki/README.md`. [Example
PR](grafana@d10549e)
mraboosk pushed a commit to mraboosk/loki that referenced this pull request Oct 7, 2024
**What this PR does / why we need it**:
Support setting labels and annotations for all services in the Helm
chart.
Move label configuration for read, write and gateway services to
#component.service.labels to improve
consistency between services.

**Which issue(s) this PR fixes**:
Fixes grafana#8627 

**Special notes for your reviewer**:

**Checklist**
- [x] Reviewed the
[`CONTRIBUTING.md`](https://github.com/grafana/loki/blob/main/CONTRIBUTING.md)
guide (**required**)
- [x] `CHANGELOG.md` updated
- [x] For Helm chart changes bump the Helm chart version in
`production/helm/loki/Chart.yaml` and update
`production/helm/loki/CHANGELOG.md` and
`production/helm/loki/README.md`. [Example
PR](grafana@2cef71e)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/helm size/L type/docs Issues related to technical documentation; the Docs Squad uses this label across many repositories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support adding annotations to all service's templates
5 participants