Skip to content
This repository has been archived by the owner on Dec 4, 2024. It is now read-only.

chore: update kommander annotations #68

Merged
merged 3 commits into from
Jan 17, 2020
Merged

Conversation

natmegs
Copy link
Contributor

@natmegs natmegs commented Jan 13, 2020

  • Update kommander monitoring addon endpoints
  • Update kommander UI endpoint
  • Add grafana annotations to kommander addon
  • Bump kommander and opsportal charts

@natmegs natmegs requested a review from gracedo January 13, 2020 23:39
@natmegs
Copy link
Contributor Author

natmegs commented Jan 13, 2020

@shaneutt can you let me know if this follows the correct contribution pattern?

endpoint.kubeaddons.mesosphere.io/kommander-grafana: "/ops/portal/kommander/monitoring/grafana"
docs.kubeaddons.mesosphere.io/thanos: "https://thanos.io/getting-started.md/"
docs.kubeaddons.mesosphere.io/karma: "https://github.com/prymitive/karma"
docs.kubeaddons.mesosphere.io/kommander-grafana: "https://grafana.com/docs/"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gracedo it's kind of hard to see in the diff but all that's changed in these files are the annotations - specifically, the endpoints (changed to the /ops/portal/kommander/monitoring/x pattern per slack), and adding annotations for kommander's grafana. Also changed the kommander UI endpoint as I think that the monitoring endpoint changes are dependent on the ui path change? Let me know if this looks right!

Copy link
Contributor

@gracedo gracedo left a comment

Choose a reason for hiding this comment

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

Let's wait on this until the charts work is all done (I was planning on modifying the annotations with the kommander bump). I was told that maybe prior to GA it was okay to modify instead of create a new revision, but I'm also not entirely sure :p

@natmegs
Copy link
Contributor Author

natmegs commented Jan 13, 2020

@gracedo Ok - I'll label this blocked for now until the charts stuff is ready

Copy link
Contributor

@gracedo gracedo left a comment

Choose a reason for hiding this comment

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

mesosphere/charts#351 was merged so we should be unblocked now!

chartReference:
chart: kommander
repo: https://mesosphere.github.io/charts/stable
version: 0.3.3
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's update this to 0.3.10 (mesosphere/charts#351)

Copy link
Contributor

Choose a reason for hiding this comment

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

make that 0.3.13, pushed up a fix :) mesosphere/charts#356

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be kept as is since we're not updating the addon itself, 0.3.13 is added for 1.182.x versions.
that said, I'm not sure if we can/need to add these annotations to 1.163.x versions, they dont even support it, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

since we're not updating the kommander chart version here, do we need this file addons/kommander/1.163.x/kommander-2.yaml?

chartReference:
chart: kommander
repo: https://mesosphere.github.io/charts/stable
version: 0.3.9
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's update this to 0.3.10 (mesosphere/charts#351)

Copy link
Contributor

Choose a reason for hiding this comment

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

^ 0.3.13

Copy link
Contributor

Choose a reason for hiding this comment

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

same comment/question as above

Copy link
Contributor

@shaneutt shaneutt left a comment

Choose a reason for hiding this comment

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

I think we should consider updating to the latest patch version as @gracedo suggested, and then ✔️

addons/kommander/1.163.x/kommander-2.yaml Outdated Show resolved Hide resolved
@juliangieseke juliangieseke mentioned this pull request Jan 15, 2020
@juliangieseke juliangieseke force-pushed the nat/kommander-addons branch 2 times, most recently from 03f8105 to f034ca1 Compare January 15, 2020 09:10
@juliangieseke
Copy link
Contributor

I took 1.173.x/kommander-2.yaml and copied it to 1.182.x/kommander-1.yaml - adding our latest version.

I can see that maintaining that many versions will get messy kinda fast…

chartReference:
chart: kommander
repo: https://mesosphere.github.io/charts/stable
version: 0.3.3
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be kept as is since we're not updating the addon itself, 0.3.13 is added for 1.182.x versions.
that said, I'm not sure if we can/need to add these annotations to 1.163.x versions, they dont even support it, right?

chartReference:
chart: kommander
repo: https://mesosphere.github.io/charts/stable
version: 0.3.9
Copy link
Contributor

Choose a reason for hiding this comment

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

same comment/question as above

addons/kommander/1.182.x/kommander-1.yaml Outdated Show resolved Hide resolved
@juliangieseke
Copy link
Contributor

tested this PR on my cluster, its running 1.182.14 and uses /ui/ url, so it seems to be fine 👍

@shaneutt shaneutt self-requested a review January 15, 2020 13:30
shaneutt
shaneutt previously approved these changes Jan 15, 2020
Copy link
Contributor

@shaneutt shaneutt left a comment

Choose a reason for hiding this comment

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

I've spoken with @juliangieseke about this in Zoom and Slack, and discussed some of the confusion related to this PR:

We are working on expanded documentation which will cover how to make releases and how to manage removing unsupported versions of an Addon which appear will be particularly helpful for Kommander which is updating rapidly and older versions are not necessarily useful to keep around.

You can track the documentation that's being created here: https://jira.mesosphere.com/browse/DCOS-62869

In the meantime, I believe we can merge this as is to get the immediate benefit of getting the patched latest revision out.

After we publish our new documentation it should be much simpler and more autonomous to do so without needing assistance from other repository code owners, @juliangieseke and I are planning on getting this fully resolved and set into motion next tuesday.

@shaneutt
Copy link
Contributor

I'm going to look into the CI issues as per DCOS-62937

@shaneutt shaneutt self-assigned this Jan 15, 2020
@shaneutt
Copy link
Contributor

So far the CI issues appear as though they may be deficiencies in teamcity resources, as I can pass the KommanderTestGroup tests locally. I'm continuing my review.

@shaneutt
Copy link
Contributor

CI may have just been flaky, as tests are now passing. Please re-review and get this one moving through.

shaneutt
shaneutt previously approved these changes Jan 15, 2020
juliangieseke
juliangieseke previously approved these changes Jan 15, 2020
Copy link
Contributor

@gracedo gracedo left a comment

Choose a reason for hiding this comment

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

Hmm, karma seems to be at the old /kommander/monitoring/karma url instead of behind /ops/portal/kommander - blocking this to investigate further

@natmegs
Copy link
Contributor Author

natmegs commented Jan 15, 2020

We are going to have another chart bump shortly for kommander and I'd like to get that in here as well before it merges.

@natmegs natmegs dismissed stale reviews from juliangieseke and shaneutt via 07124f2 January 15, 2020 21:33
@natmegs
Copy link
Contributor Author

natmegs commented Jan 15, 2020

I added a new commit to change from kommander 1.182.x to 1.184.x and to also update the opsportal chart. This is now blocked on mesosphere/charts#361

@juliangieseke
Copy link
Contributor

juliangieseke commented Jan 16, 2020

I think it was also blocked on mesosphere/charts#360 - I've merged that and tested this branch on my cluster. thanos & grafana are accessible on /ops/portal/kommander/{query, grafana} paths 👍 Lets get this in 💯

juliangieseke
juliangieseke previously approved these changes Jan 16, 2020
@juliangieseke
Copy link
Contributor

@gracedo can you please unblock this pr? 🙇

@gracedo
Copy link
Contributor

gracedo commented Jan 16, 2020

@juliangieseke just starting up a cluster now to check. or - if you still have yours up running this version, can you check /ops/portal/kommander/karma?

@juliangieseke
Copy link
Contributor

I've cleaned up this PR to match the one for 1.3.0-rc.1 branch (#84). @jimmidyson kept your commit, but its still broken

@juliangieseke juliangieseke merged commit cff56ac into master Jan 17, 2020
@juliangieseke juliangieseke deleted the nat/kommander-addons branch January 17, 2020 13:12
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants