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

feat: Add serviceMonitor resource #537

Merged
merged 10 commits into from
Aug 22, 2024

Conversation

shubham-cmyk
Copy link
Contributor

@shubham-cmyk shubham-cmyk commented Apr 24, 2024

Description

It would add a prometheus serviceMonitor resource.

Check List

  • Commits are signed per the DCO using --signoff

For any changes to files within Helm chart directories:

  • Helm chart version bumped
  • Helm chart CHANGELOG.md updated to reflect change

PR sponsored by Obmondo.com

@smbambling
Copy link

Is this inteded to be used in conjunction with https://github.com/Aiven-Open/prometheus-exporter-plugin-for-opensearch/tree/main ?

If so might I suggest setting the path to "/_prometheus/metrics"

Copy link
Member

@TheAlgo TheAlgo left a comment

Choose a reason for hiding this comment

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

Thanks for raising the PR. Can you look at lint failures once? Also is there an Github issue for this feature? It will be great if we can have one.

@shubham-cmyk
Copy link
Contributor Author

@TheAlgo
Bumped the chart version can you run the check again

@shubham-cmyk shubham-cmyk requested a review from TheAlgo May 27, 2024 18:21
@eyenx
Copy link
Contributor

eyenx commented Jul 1, 2024

We would love to have this feature in the chart!

@gaiksaya
Copy link
Member

gaiksaya commented Jul 1, 2024

Hi @shubham-cmyk
Can you please rebase? Looks like branch has a conflict.

@eyenx
Copy link
Contributor

eyenx commented Aug 14, 2024

@shubham-cmyk do you need any help with this? we are looking forward to have this feature in the chart.

@peterzhuamazon
Copy link
Member

I will take a look tomorrow and see if I can help rebase.

Thanks.

@peterzhuamazon
Copy link
Member

I updated the version and changelog and tried to push but not able to do so, seems like the repo belongs to an org:

$ git push origin serviceMonitor
ERROR: Permission to Obmondo/helm-charts-1.git denied to peterzhuamazon.
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.

Hi @shubham-cmyk could you please update the PR?

Thanks.

@VILJkid
Copy link
Contributor

VILJkid commented Aug 16, 2024

Hi team,

I'll be picking up where @shubham-cmyk left off to address the recent changes. I'll update the PR shortly with the necessary adjustments. Thanks for your patience, and I’ll keep you posted on the progress!

@eyenx
Copy link
Contributor

eyenx commented Aug 16, 2024

Thanks @VILJkid

@VILJkid
Copy link
Contributor

VILJkid commented Aug 16, 2024

@eyenx, I’ve taken care of the rebase and chart version bump. @shubham-cmyk is on vacation, so please review the changes and inform me if any additional updates are needed.

@VILJkid VILJkid force-pushed the serviceMonitor branch 2 times, most recently from 74386cb to b608181 Compare August 16, 2024 09:30
@eyenx
Copy link
Contributor

eyenx commented Aug 20, 2024

@TheAlgo can we merge this?

Shubham Gupta and others added 5 commits August 21, 2024 15:34
Signed-off-by: Shubham Gupta <sgupta@obmondo.com>
Signed-off-by: VILJkid <sidvjmostwanted@gmail.com>
Signed-off-by: Shubham Gupta <sgupta@obmondo.com>
Signed-off-by: VILJkid <sidvjmostwanted@gmail.com>
Signed-off-by: VILJkid <sidvjmostwanted@gmail.com>
Signed-off-by: VILJkid <sidvjmostwanted@gmail.com>
Signed-off-by: VILJkid <sidvjmostwanted@gmail.com>
Signed-off-by: VILJkid <sidvjmostwanted@gmail.com>
Signed-off-by: VILJkid <sidvjmostwanted@gmail.com>
@VILJkid
Copy link
Contributor

VILJkid commented Aug 21, 2024

The requested changes have been pushed. Kindly review and merge this feature.

Thanks for your time,
@prudhvigodithi @eyenx @bbarani @DandyDeveloper @peterzhuamazon @gaiksaya @TheAlgo

Copy link
Member

@TheAlgo TheAlgo left a comment

Choose a reason for hiding this comment

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

Can you check lint failures before reviewers can take a look?

Signed-off-by: VILJkid <sidvjmostwanted@gmail.com>
Signed-off-by: VILJkid <sidvjmostwanted@gmail.com>
@VILJkid
Copy link
Contributor

VILJkid commented Aug 21, 2024

Thanks @TheAlgo!

The lint failures, and changelog version comparison is fixed!

charts/opensearch-dashboards/CHANGELOG.md Outdated Show resolved Hide resolved
charts/opensearch-dashboards/Chart.yaml Outdated Show resolved Hide resolved
charts/opensearch/CHANGELOG.md Outdated Show resolved Hide resolved
charts/opensearch/Chart.yaml Outdated Show resolved Hide resolved
charts/opensearch/CHANGELOG.md Outdated Show resolved Hide resolved
charts/opensearch-dashboards/CHANGELOG.md Outdated Show resolved Hide resolved
Signed-off-by: VILJkid <sidvjmostwanted@gmail.com>
@VILJkid
Copy link
Contributor

VILJkid commented Aug 21, 2024

Thanks @peterzhuamazon!

The fix has been pushed and the minor versions have been bumped up, as per the change request.

Copy link
Member

@peterzhuamazon peterzhuamazon left a comment

Choose a reason for hiding this comment

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

Hi @shubham-cmyk @VILJkid ,

I am fine with this PR now.
Giving my approval.
Waiting for @prudhvigodithi @TheAlgo to do another pass before merging.
Once this is merged if you could help create another backport to 1.x branch, that would be great to have.

Thanks for contribution.

@prudhvigodithi
Copy link
Member

Thanks @shubham-cmyk @VILJkid @peterzhuamazon @TheAlgo , LGTM we can merge once the CI checks pass.

@peterzhuamazon peterzhuamazon dismissed TheAlgo’s stale review August 22, 2024 18:03

Since we have 2 approvals and addressed @TheAlgo initial comment, will merge now. Thanks.

@peterzhuamazon peterzhuamazon merged commit 4253842 into opensearch-project:main Aug 22, 2024
8 checks passed
@peterzhuamazon
Copy link
Member

Thanks @shubham-cmyk @VILJkid ,

Would you be able to backport this change to 1.x branch?

Thanks!

@VILJkid
Copy link
Contributor

VILJkid commented Aug 23, 2024

Thanks @peterzhuamazon,

Yes, I'll raise another PR with this change for branch 1.x shortly.

@VILJkid
Copy link
Contributor

VILJkid commented Aug 23, 2024

Hi @peterzhuamazon,

Here's the PR for backporting this change to 1.x : #578

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

8 participants