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

Fix servicemonitor #248

Merged
merged 5 commits into from
Jan 20, 2023
Merged

Fix servicemonitor #248

merged 5 commits into from
Jan 20, 2023

Conversation

ndegory
Copy link
Contributor

@ndegory ndegory commented Jan 6, 2023 •
edited
Loading

what

  • Fix the servicemonitor when basic auth is enabled
  • Add clarification that prometheus metrics have to be enabled in the repoConfig
  • Use the chart appVersion as default tag
  • Build cleanup
  • Update documentation
  • Bump chart patch version

why

  • metrics were missing, and TargetDown was triggered for the Atlantis service

tests

  • I have tested my changes by modifying the servicemonitor from a previous version of the Atlantis chart

  • checked the servicemonitor rendered by this PR by running helm template, with different options set in servicemonitor.auth

  • checked the image tag change:

    ➜ helm template atlantis ./charts/atlantis | ag image:
          image: "ghcr.io/runatlantis/atlantis:v0.22.3"
      image: dduportal/bats:0.4.0
      image: lachlanevenson/k8s-kubectl:v1.4.8-bash

references

@ndegory ndegory requested a review from a team as a code owner January 6, 2023 01:57
nitrocode
nitrocode reviewed Jan 6, 2023
View reviewed changes
charts/atlantis/Chart.yaml
description: A Helm chart for Atlantis https://www.runatlantis.io
name: atlantis
version: 4.9.2
version: 4.9.3
keywords:
Copy link
Member

Choose a reason for hiding this comment

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

The app version should bump the minor version to keep it consistent with previous helm chart releases. We should do this in a separate pr

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, will remove it

Copy link
Member

Choose a reason for hiding this comment

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

Let's just bump the minor version since the changes youre doing are still present, no?

Ref https://github.com/runatlantis/helm-charts/pull/221/files

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if this will break existing functionality so I will wait for others to review this PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated the patch version, not sure if the changes in the values.yaml file require an update of the minor version.

Teko012
Teko012 reviewed Jan 6, 2023
View reviewed changes
charts/atlantis/values.yaml Outdated
Comment on lines 432 to 440
servicemonitor:
enabled: false
interval: "30s"
Copy link
Contributor

@Teko012 Teko012 Jan 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Could you add these to the readme as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@ndegory ndegory mentioned this pull request Jan 10, 2023
Closed
@ndegory ndegory force-pushed the 247-fix-servicemonitor-labels-selector branch from a8e9c51 to b09c9a9 Compare January 11, 2023 00:27
@ndegory ndegory changed the title Fix servicemonitor labels selector Fix servicemonitor Jan 11, 2023
@ndegory
Copy link
Contributor Author

ndegory commented Jan 11, 2023

@nitrocode the PR is ready for review

@GenPage
Copy link
Member

GenPage commented Jan 20, 2023

@ndegory Please rebase in the meantime, I approved the tests and linters to run.

@ndegory ndegory force-pushed the 247-fix-servicemonitor-labels-selector branch from b09c9a9 to d0a8cca Compare January 20, 2023 15:00
@ndegory
Copy link
Contributor Author

ndegory commented Jan 20, 2023

@GenPage I rebased the PR and bumped the chart version.

GenPage
GenPage approved these changes Jan 20, 2023
View reviewed changes
Copy link
Member

@GenPage GenPage left a comment

Choose a reason for hiding this comment

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

Thank you, we appreciate the additional tweaks to CI and defaulting the image tag.

@GenPage GenPage merged commit 859e646 into runatlantis:main Jan 20, 2023
@nitrocode nitrocode mentioned this pull request Jan 26, 2023
Closed
@nitrocode nitrocode mentioned this pull request May 24, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Teko012 Teko012 Teko012 left review comments

@nitrocode nitrocode nitrocode left review comments

@GenPage GenPage GenPage approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ndegory @GenPage @nitrocode @Teko012