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: add kubernetes monitor sub-chart #354

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

liam-mackie
Copy link
Contributor

This PR adds the kubernetes monitor to the agent as a helm dependency, and defaults the installation to false. This allows us to (in future) allow anyone using the agent to also enable the monitor.

@liam-mackie liam-mackie requested a review from a team as a code owner December 5, 2024 22:57
Copy link

changeset-bot bot commented Dec 5, 2024

🦋 Changeset detected

Latest commit: 16a57f8

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
kubernetes-agent Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@@ -0,0 +1,5 @@
---
"kubernetes-agent": minor
Copy link
Contributor

Choose a reason for hiding this comment

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

Kinda devils advocate, but should this be a major bump?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's backwards compatible, but adding new functionality, so left it as a minor according to SemVer.

Copy link
Contributor

Choose a reason for hiding this comment

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

yep, happy with that. Just wanted to ask the question :)

# Used to enable the Kubernetes Monitor
# Do not use this, as it is not supported at the moment
# @ignored
kubernetes-monitor-chart:
Copy link
Contributor

Choose a reason for hiding this comment

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

could just be kubernetesMonitor?
The fact it's a chart is kinda irrelevant for the values?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When setting values, the sub-charts name is the root of the values for that particular chart. This allows us to expose (in future) the documentation for the subchart in our readme. If we use kubernetesMonitor, we'd need to name the chart kubernetesMonitor, which clashes with the actual image 😅

Copy link
Contributor

Choose a reason for hiding this comment

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

gotcha 👍

@@ -0,0 +1,6 @@
dependencies:
- name: kubernetes-monitor-chart
repository: oci://docker.io/octopusdeploy
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to change the repo this chart is pulled from? (say if I want to pull from Artifactory?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, though this is only referenced once during build time, so I didn't see a reason to make it configurable. The build downloads all sub-charts (note the Chart.lock), then packages all subcharts into the helm chart as part of the packaging process.

Copy link
Contributor

Choose a reason for hiding this comment

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

kk, we can always change it if we need to in the future 👍

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

Successfully merging this pull request may close these issues.

2 participants