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

k8s/topgun: update prometheus dependencies and disable worker #5568

Merged
merged 1 commit into from
May 11, 2020

Conversation

cirocosta
Copy link
Member

@cirocosta cirocosta commented May 7, 2020

Why is this PR needed?

With stable/prometheus going from having kube-state-metrics embedded in its set of templates to a separate dependency, k8s-topgun started failing:

Error: found in requirements.yaml, but missing in charts/ directory: kube-state-metrics

Without this PR, we can't have k8s-topgun running without having the version of helm/charts pinned to an old commit, which would force us to have that commit hardcoded in the release branches, as well as the main pipeline.

What is this PR trying to accomplish?

It tries to make the installation of prometheus possible regardless of how its dependencies are set up (either through requirements.yaml - the new way of doing thing -, or the old one - embedded).

How does it accomplish that?

It does so by running helm dependency update before running helm install.

Given that such command will not fail in case of requirements.yaml not existing (which would be true for older versions of the helm/charts repo), it's safe to always do it.

Reviewer Checklist

  • Code reviewed
  • Tests reviewed
  • Documentation reviewed
  • Release notes reviewed
  • PR acceptance performed
  • New config flags added? Ensure that they are added to the BOSH
    and Helm packaging; otherwise, ignored for the integration tests (for example, if they are Garden configs that are not displayed in the --help text).

the prometheus chart recently updated itself to have
`kube-state-metrics` being a subchart, rather than embedded on itself.

because of that, we need to first fetch its dependencies, and then later
on, install it.

here I also got rid of the Concourse worker that was being deployed -
this is possible because nowadays we offer the ability to do so
(something you couldn't before), and because the worker is unecessary
for this case.

Signed-off-by: Ciro S. Costa <cscosta@pivotal.io>
@cirocosta
Copy link
Member Author

I just realized this is very related to concourse/ci#285.

IMO both coexist: I don't think that because this solves the point of having to pin that the other issue would be invalidated - it'd still be great to keep track of the versions to establish the matrix.

Copy link
Member

@taylorsilva taylorsilva left a comment

Choose a reason for hiding this comment

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

ran the test and it passed! Made sure my local helm-charts repo was on the latest master

@cirocosta cirocosta merged commit 313b9c6 into master May 11, 2020
@cirocosta cirocosta deleted the k8s-topgun-prometheus-fix branch May 11, 2020 20:38
@taylorsilva taylorsilva added the release/no-impact This is an issue that never affected released versions (i.e. a regression caught prior to shipping). label May 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release/no-impact This is an issue that never affected released versions (i.e. a regression caught prior to shipping).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants