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

refactor(kommander): Use thanos & karma source code as subcharts #453

Merged
merged 17 commits into from
Mar 19, 2020
Merged

refactor(kommander): Use thanos & karma source code as subcharts #453

merged 17 commits into from
Mar 19, 2020

Conversation

samvantran
Copy link
Contributor

@samvantran samvantran commented Feb 24, 2020 •
edited
Loading

What changes are proposed in this pull request?

Resolves: D2IQ-63905

This PR copies the chart src code from kommander-thanos and kommander-karma into kommander's charts/ folder. This will help reduce the amount of PRs needed when updating the charts.

Previously if we were to make a change in, say, Karma we'd have to:

  • create a PR to update the kommander-karma code and bump karma version
  • create a PR to bump the kommander chart version and update karma tarball
  • create a PR in kubeaddons-kommander repo to bump the kommander chart version

By porting the kommander-karma and -thanos code, we can reduce by one step and make updates and bump the kommander chart version in the same PR. This speeds up development and reduces PR reviews for code changes that were already approved.

Notes:

  • A lot of files shown in the PR but it's really just copy+paste. The only changes I made were in the last two commits.
  • I'm mimicking istio subcharts
  • The code is ready but I'm marking as wip for now until post-Kommander GA

How were these changes tested?

  • same way as described in refactor: simplify kommander's grafana dashboard job #433 except I deployed Konvoy with cluster.yaml 
      - configRepository: https://github.com/samvantran/kubeaddons-kommander
    configVersion: combine-central-monitoring
    • checked Thanos and Karma addons deployed normally and dashboards were up and running
  • ran this Konvoy CI job to test CI against the addon changes

@samvantran samvantran added the wip label Feb 24, 2020
@samvantran samvantran requested a review from a team as a code owner February 24, 2020 07:15
@shaneutt
Copy link
Contributor

@samvantran please don't hesitate to ask @mesosphere/sig-ksphere-catalog for any assistance you might need moving this from wip to ready!

@shaneutt shaneutt added ready ready and removed wip labels Mar 11, 2020
@shaneutt shaneutt requested review from a team as code owners March 19, 2020 14:07
shaneutt
shaneutt previously approved these changes Mar 19, 2020
View reviewed changes
@shaneutt shaneutt requested a review from juliangieseke March 19, 2020 14:08
juliangieseke
juliangieseke approved these changes Mar 19, 2020
View reviewed changes
Copy link
Contributor

@juliangieseke juliangieseke left a comment

Choose a reason for hiding this comment

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

LGTM 🎉

would it be possible to get rid of these subcharts in the first place? Like integrating their stuff into kommander chart? seems like its a somewhat unnecessary level of indirection now. not sure if that assumption holds true tho.

@juliangieseke
Copy link
Contributor

CI says no :(

Error: requirements.lock is out of sync with requirements.yaml

juliangieseke
juliangieseke suggested changes Mar 19, 2020
View reviewed changes
Copy link
Contributor

@juliangieseke juliangieseke left a comment

Choose a reason for hiding this comment

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

subcharts are out of date. they need to be updated with latest versions from this repos master.

stable/kommander/charts/kommander-karma/Chart.yaml Outdated
@@ -0,0 +1,9 @@
apiVersion: v1
appVersion: "1.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

actually, this is out of date.

stable/kommander/charts/kommander-thanos/Chart.yaml Outdated
@@ -0,0 +1,9 @@
apiVersion: v1
appVersion: "1.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

same here.

@samvantran
update subcharts with latest changes
95c8f0c 
This updates the kommander-thanos/karma subcharts with the latest
changes from ../stable folder (incl. portalRBAC)

Also updates requirements.lock
@samvantran samvantran force-pushed the combine-central-monitoring branch from 809eb19 to 5cf8cc7 Compare March 19, 2020 17:47
juliangieseke
juliangieseke approved these changes Mar 19, 2020
View reviewed changes
Copy link
Contributor

@juliangieseke juliangieseke left a comment

Choose a reason for hiding this comment

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

Sorry for not catching this earlier: shouldnt the old ones being removed in the same PR? Like actually moved instead of copied?

@samvantran
Copy link
Contributor Author

Sorry for not catching this earlier: shouldnt the old ones being removed in the same PR? Like actually moved instead of copied?

Didn't want to do too much in one PR. If it's all right with you, would rather make that a separate task.

@shaneutt shaneutt merged commit 721d64e into mesosphere:master Mar 19, 2020
mesosphere-teamcity pushed a commit that referenced this pull request Mar 19, 2020
refactor(kommander): Use thanos & karma source code as subcharts (#453)
7232ca5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gracedo gracedo gracedo approved these changes

@shaneutt shaneutt shaneutt approved these changes

@juliangieseke juliangieseke juliangieseke approved these changes

@branden branden Awaiting requested review from branden

@GoelDeepak GoelDeepak Awaiting requested review from GoelDeepak

Assignees

@samvantran samvantran

Labels
ready ready
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@samvantran @shaneutt @juliangieseke @gracedo @alejandroEsc