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(woodpecker-server)!: add monitoring resources and other improvements #94

Merged
merged 6 commits into from
Oct 17, 2023

Conversation

genofire
Copy link
Contributor

@genofire genofire commented Oct 13, 2023

@pat-s please review this time faster.

#22

fix #21


some things already merged by #77


Changelog

BREAKING CHANGES

  • split metrics.enabled and the PodMonitor deployment to prometheus.podmonitor.enabled
    • move metrics.interval to prometheus.podmonitor.interval

Features

  • metrics port also on services
  • prometheus.podmonitor.interval optional (for use global server value)
  • prometheus.podmonitor.label for dedicated labels only on pod-monitor
  • first prometheus rule with alerting (is there a worker)
  • prepare to ship grafana-dashboard

Bugfixes

chore

  • replace if with with
  • use named ports in services
  • remove metrics.path (user should not change that value)
  • remove not used ingress.hosts.paths.backend from values.yaml

Copy link
Collaborator

@pat-s pat-s left a comment

Choose a reason for hiding this comment

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

Thanks! See comments inline.

A short intro/example in the README would be great - most know Prometheus but it would be good to a short bit of information what this chart actually provides after this PR.

And please remove charts/server/grafana_dashboards/.gitkeep - it seemed to have slipped in by accident?

charts/server/templates/statefulset.yaml Outdated Show resolved Hide resolved
charts/server/templates/servicemonitor.yaml Outdated Show resolved Hide resolved
charts/server/values.yaml Outdated Show resolved Hide resolved
@genofire
Copy link
Contributor Author

charts/server/grafana_dashboards/.gitkeep was created, because otherwise git does not create this directory charts/server/grafana_dashboards/, because there are not yet any files inside.

Of course i could delete the support of grafana_dashboards (and prometheus-rules, because we did not have any dashboards (and rules) yet.

or i need

@qwerty287 qwerty287 added the feature 🚀️ Add new feature label Oct 13, 2023
@genofire
Copy link
Contributor Author

wow for the fast review (multiply times a day, my last just get a "please rebase" after four mouth).

i do not know, how you create / generate changelogs. It does not look like convention-commit based ... so i write some in the description.

@genofire genofire force-pushed the patch-1 branch 6 times, most recently from 51b688d to 0583b81 Compare October 13, 2023 23:44
@genofire genofire changed the title feat(woodpecker-server): add monitoring resources feat(woodpecker-server)!: add monitoring resources and other improvements Oct 13, 2023
@pat-s
Copy link
Collaborator

pat-s commented Oct 14, 2023

wow for the fast review (multiply times a day, my last just get a "please rebase" after four mouth).

I am just contributing since some months - I wasn't (really) active during your last PR.

i do not know, how you create / generate changelogs.

It's done automatically by a release bot - which scrapes the commit message/PR titles and merges them with the labels assigned.

But for this one we should add a upgrade changelog in the README - so it would be good to do it there, i.e. in README-main.md.gotmpl. It only needs to cover instructions for users - doesn't need to be a full changelog :) Just focus on this PR and the "breaking" changes.

@genofire
Copy link
Contributor Author

So should i split this PR in several PRs to get a nice CHANGELOG like in my description.

@pat-s
Copy link
Collaborator

pat-s commented Oct 14, 2023

Can you please add a new section to the README (below "Versioning") as follows

## Upgrading

<details>

<summary>To 1.0.0</summary>

YOUR CONTENT HERE

</details>

and replace YOUR CONTENT HERE with the user-facing upgrade notes? That would be awesome. No need for another PR, can go in here.

No features, just what users need to change if they were already using parts of the old implementation. Features etc. go into the release changelog then.

@genofire genofire force-pushed the patch-1 branch 3 times, most recently from 467bf79 to e8dcf5c Compare October 15, 2023 13:49
@genofire
Copy link
Contributor Author

done, i have also improve the with/if on a place in the agent (but not change any logic)

Chart.lock Outdated Show resolved Hide resolved
README-main.md.gotmpl Outdated Show resolved Hide resolved
Copy link
Collaborator

@pat-s pat-s left a comment

Choose a reason for hiding this comment

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

Thanks! A few last comments:

  • Please revert the version upgrades (they are done during the release and are also inconsistent in this PR)
  • Please avoid force pushes for future PRs as this makes it hard to see what changed (I remember you did this also in other/previous PRs - it's quite uncommon to do so in a regular PR workflow)
  • Please merge main cleanly, then the dind removal changes should not appear in the PR

Besides: thanks for contributing 👍

@genofire
Copy link
Contributor Author

genofire commented Oct 15, 2023

  1. done
  2. sorry, i understand your workflow (with squashing on merge) now
  3. i use the main branch (looks like dind PR has not generate docs) ... should i go back in commits?

genofire and others added 2 commits October 15, 2023 22:34
Co-authored-by: Patrick Schratz <patrick.schratz@gmail.com>
Copy link
Collaborator

@pat-s pat-s left a comment

Choose a reason for hiding this comment

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

Almost there :)

Chart.lock Outdated Show resolved Hide resolved
charts/server/Chart.yaml Outdated Show resolved Hide resolved
README-main.md.gotmpl Show resolved Hide resolved
charts/server/values.yaml Show resolved Hide resolved
charts/server/values.yaml Show resolved Hide resolved
@genofire genofire force-pushed the patch-1 branch 5 times, most recently from 96c9559 to 839f974 Compare October 17, 2023 17:07
Copy link
Collaborator

@pat-s pat-s left a comment

Choose a reason for hiding this comment

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

Thanks again! 🚀

@pat-s pat-s merged commit 174aa98 into woodpecker-ci:main Oct 17, 2023
2 checks passed
@woodpecker-bot woodpecker-bot mentioned this pull request Nov 13, 2023
1 task
@genofire
Copy link
Contributor Author

genofire commented Nov 17, 2023

when does it released?

@pat-s
Copy link
Collaborator

pat-s commented Nov 18, 2023

When WP 2.0 gets released

@woodpecker-bot
Copy link
Collaborator

🎉 This PR is included in version 1.0.0 🎉

The release is now available here

Thank you for your contribution. ❤️📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Prometheus Operator CRDs - ServiceMonitor
4 participants