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.

Sign up for GitHub

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

Jump to bottom

Add support for service monitors' custom labels in Scylla Helm chart #2037

Merged
merged 2 commits into from
Oct 30, 2024
Merged

Add support for service monitors' custom labels in Scylla Helm chart #2037

merged 2 commits into from
Oct 30, 2024

Conversation

TessaIO
Copy link
Contributor

@TessaIO TessaIO commented Jul 26, 2024

Description of your changes:
This PR aims to support custom labels for service monitors created by the Helm chart. This is needed because we only consider Prometheus service monitors with a specific label and currently service monitors created by the Scylla Helm chart don't have any label.

Which issue is resolved by this Pull Request:
Resolves #1194

@scylla-operator-bot scylla-operator-bot bot added do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Jul 26, 2024
@scylla-operator-bot scylla-operator-bot
Copy link
Contributor

Hi @TessaIO. Thanks for your PR.

I'm waiting for a scylladb member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@scylla-operator-bot scylla-operator-bot bot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 26, 2024
@TessaIO TessaIO force-pushed the feat-support-custom-labels-for-service-monitors branch from 55615c4 to fd11bd9 Compare July 26, 2024 09:13
@scylla-operator-bot scylla-operator-bot
Copy link
Contributor

The Scylla Operator project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

  • After 30d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

  • Mark this PR as fresh with /remove-lifecycle stale
  • Close this PR with /close
  • Offer to help out

/lifecycle stale

@scylla-operator-bot scylla-operator-bot bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Aug 25, 2024
@TessaIO
Copy link
Contributor Author

TessaIO commented Aug 25, 2024

/remove-lifecycle stale

@scylla-operator-bot scylla-operator-bot bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Aug 25, 2024
@TessaIO
Copy link
Contributor Author

TessaIO commented Aug 26, 2024

@tnozicka can you please take a look at it?

@scylla-operator-bot scylla-operator-bot
Copy link
Contributor

The Scylla Operator project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

  • After 30d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

  • Mark this PR as fresh with /remove-lifecycle stale
  • Close this PR with /close
  • Offer to help out

/lifecycle stale

@scylla-operator-bot scylla-operator-bot bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 25, 2024
@TessaIO
Copy link
Contributor Author

TessaIO commented Sep 25, 2024

/remove-lifecycle stale

@scylla-operator-bot scylla-operator-bot bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 25, 2024
@scylla-operator-bot scylla-operator-bot
Copy link
Contributor

The Scylla Operator project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

  • After 30d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

  • Mark this PR as fresh with /remove-lifecycle stale
  • Close this PR with /close
  • Offer to help out

/lifecycle stale

@scylla-operator-bot scylla-operator-bot bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Oct 26, 2024
@TessaIO
Copy link
Contributor Author

TessaIO commented Oct 26, 2024

/remove-lifecycle stale

@scylla-operator-bot scylla-operator-bot bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Oct 26, 2024
@TessaIO
Copy link
Contributor Author

TessaIO commented Oct 26, 2024

ping @tnozicka

zimnx
zimnx approved these changes Oct 28, 2024
View reviewed changes
Copy link
Collaborator

@zimnx zimnx left a comment

Choose a reason for hiding this comment

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

thanks, please extract autogenerated changes (schema) to separate commit and rename your commits/PR title to use imperative mood.

@scylla-operator-bot scylla-operator-bot bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 28, 2024
@TessaIO TessaIO force-pushed the feat-support-custom-labels-for-service-monitors branch from fd11bd9 to 81eb656 Compare October 28, 2024 08:40
@scylla-operator-bot scylla-operator-bot bot added do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 28, 2024
@TessaIO TessaIO force-pushed the feat-support-custom-labels-for-service-monitors branch from 297a10c to 8197b68 Compare October 28, 2024 08:49
@scylla-operator-bot scylla-operator-bot bot removed the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 28, 2024
@rzetelskik
Copy link
Member

rzetelskik commented Oct 28, 2024 •
edited
Loading

@zimnx thx for the review. Commits are split now, but I'm unsure what you mean by imperative mood. I followed the contribution guide anyway. If it doesn't still comply please suggest one

The contribution guide is a bit outdated, @zimnx is working on updating it in #2130.
See https://cbea.ms/git-commit/#imperative; the commit subject line and PR title should basically answer If applied, this commit/PR will ..., e.g. Add support for service monitors' custom labels in Scylla Helm chart.

As per #2037 (comment), it also shouldn't contain the mentioned keywords. The commit containing the autogenerated changes should only say Update autogenerated or the like.

@TessaIO TessaIO changed the title feat/helm: support custom labels for service monitors Add support for service monitors' custom labels in Scylla Helm chart Oct 28, 2024
@TessaIO TessaIO force-pushed the feat-support-custom-labels-for-service-monitors branch 2 times, most recently from a5faf5a to 157ed50 Compare October 28, 2024 09:44
@scylla-operator-bot scylla-operator-bot bot removed the do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. label Oct 28, 2024
@TessaIO
Copy link
Contributor Author

TessaIO commented Oct 28, 2024

@zimnx @rzetelskik thx, please check!

@TessaIO TessaIO force-pushed the feat-support-custom-labels-for-service-monitors branch 2 times, most recently from 7376130 to f16c078 Compare October 28, 2024 09:47
@TessaIO TessaIO changed the title Add support for service monitors' custom labels in Scylla Helm chart If applied, this PR will Add support for service monitors' custom labels in Scylla Helm chart Oct 28, 2024
@TessaIO TessaIO requested a review from zimnx October 28, 2024 09:48
@rzetelskik
Copy link
Member

@TessaIO thanks, but I only meant that it should answer If applied, this commit/PR will..., not include it. 😅
Please change both the PR title and commit subject line to only Add support for service monitors' custom labels in Scylla Helm chart and it should be good to go.

/ok-to-test
/priority important-longterm
/kind machinery

@scylla-operator-bot scylla-operator-bot bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. kind/machinery Categorizes issue or PR as related to Makefile, scripts or similar changes. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Oct 28, 2024
@TessaIO TessaIO changed the title If applied, this PR will Add support for service monitors' custom labels in Scylla Helm chart Add support for service monitors' custom labels in Scylla Helm chart Oct 28, 2024
@TessaIO TessaIO force-pushed the feat-support-custom-labels-for-service-monitors branch from f16c078 to 406d764 Compare October 28, 2024 10:48
@TessaIO
Copy link
Contributor Author

TessaIO commented Oct 28, 2024

@rzetelskik done!

@zimnx
Copy link
Collaborator

zimnx commented Oct 28, 2024

Looks like you haven't fully updated what's autogenerated. Run make update in root dir of repository and squash into autogenerated commit.

@TessaIO
Update autogenerated
ebb339f 
Signed-off-by: TessaIO <[email protected]>
Signed-off-by: AhmedGrati <[email protected]>
@TessaIO TessaIO force-pushed the feat-support-custom-labels-for-service-monitors branch from 406d764 to ebb339f Compare October 29, 2024 21:09
@TessaIO
Copy link
Contributor Author

TessaIO commented Oct 29, 2024

@zimnx can you please take a look?

zimnx
zimnx approved these changes Oct 30, 2024
View reviewed changes
Copy link
Collaborator

@zimnx zimnx 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 contribution!

@scylla-operator-bot scylla-operator-bot bot added the lgtm Indicates that a PR is ready to be merged. label Oct 30, 2024
@scylla-operator-bot scylla-operator-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: TessaIO, zimnx

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@scylla-operator-bot scylla-operator-bot bot merged commit 24bf4fc into scylladb:master Oct 30, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zimnx zimnx zimnx approved these changes

@rzetelskik rzetelskik Awaiting requested review from rzetelskik

Assignees

@zimnx zimnx

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/machinery Categorizes issue or PR as related to Makefile, scripts or similar changes. lgtm Indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add support for service monitors' custom labels in Scylla Helm chart
3 participants
@TessaIO @rzetelskik @zimnx