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 helm chart #318

Merged
merged 10 commits into from
Apr 27, 2023
Merged

feat: add helm chart #318

merged 10 commits into from
Apr 27, 2023

Conversation

TylerGillson
Copy link
Contributor

@TylerGillson TylerGillson commented Apr 21, 2023

📑 Description

Replace static manifests with a Helm chart & update Makefile accordingly

Closes #307

@TylerGillson TylerGillson requested review from a team as code owners April 21, 2023 21:21
@TylerGillson TylerGillson changed the title feat: Add helm chart feat: add helm chart Apr 21, 2023
@TylerGillson TylerGillson force-pushed the tyler/helm-chart branch 2 times, most recently from dc2787f to a4bf7cc Compare April 21, 2023 21:28
@arbreezy
Copy link
Member

@TylerGillson can you also add a reference to this GH issue in your PR, please?

@TylerGillson TylerGillson changed the title feat: add helm chart feat: add helm chart (#307) Apr 24, 2023
@TylerGillson TylerGillson changed the title feat: add helm chart (#307) feat: add helm chart Apr 24, 2023
@arbreezy
Copy link
Member

@TylerGillson , I think it's nice to have appVersion specifying the container image version and also have a way to overwrite it if a user wants to in their own values.yaml or by explicitly setting it

e.g
"{{ .Values.image.repository }}:{{ .Values.image.tag | default .Chart.AppVersion }}"

what do you think?

@arbreezy arbreezy requested review from a team and removed request for panpan0000 April 24, 2023 16:52
@TylerGillson
Copy link
Contributor Author

TylerGillson commented Apr 24, 2023

@TylerGillson , I think it's nice to have appVersion specifying the container image version and also have a way to overwrite it if a user wants to in their own values.yaml or by explicitly setting it

e.g "{{ .Values.image.repository }}:{{ .Values.image.tag | default .Chart.AppVersion }}"

what do you think?

@arbreezy @matthisholleville I was trying to stick with the K8s common label conventions. How about introducing app.k8sgpt.image instead?

Output of helm template k8sgpt charts/k8sgpt:

apiVersion: apps/v1
kind: Deployment
metadata:
  name: k8sgpt
  namespace: "default"
  labels:
    helm.sh/chart: k8sgpt-1.0.0
    app.kubernetes.io/name: k8sgpt
    app.kubernetes.io/instance: k8sgpt
    app.kubernetes.io/managed-by: Helm
    app.kubernetes.io/version: "0.2.4"
    app.k8sgpt.image: "ghcr.io/k8sgpt-ai/k8sgpt:v0.2.4"

@arbreezy
Copy link
Member

@TylerGillson , I think it's nice to have appVersion specifying the container image version and also have a way to overwrite it if a user wants to in their own values.yaml or by explicitly setting it
e.g "{{ .Values.image.repository }}:{{ .Values.image.tag | default .Chart.AppVersion }}"
what do you think?

@arbreezy @matthisholleville I was trying to stick with the K8s common label conventions. How about introducing app.k8sgpt.image instead?

Output of helm template k8sgpt charts/k8sgpt:

apiVersion: apps/v1
kind: Deployment
metadata:
  name: k8sgpt
  namespace: "default"
  labels:
    helm.sh/chart: k8sgpt-1.0.0
    app.kubernetes.io/name: k8sgpt
    app.kubernetes.io/instance: k8sgpt
    app.kubernetes.io/managed-by: Helm
    app.kubernetes.io/version: "0.2.4"
    app.k8sgpt.image: "ghcr.io/k8sgpt-ai/k8sgpt:v0.2.4"

I was talking about in deployment's template, how you could set the container image version, which is something quite common with OSS charts.

@TylerGillson
Copy link
Contributor Author

@arbreezy gotcha! Made the change you suggested now that I've understood you 😅

@arbreezy
Copy link
Member

@AlexsJones, do we need a release-please annotation since appVersion will point to our latest released container image?

@thschue
Copy link
Contributor

thschue commented Apr 26, 2023

@AlexsJones, do we need a release-please annotation since appVersion will point to our latest released container image?

I think this would make sense

@arbreezy
Copy link
Member

@AlexsJones, do we need a release-please annotation since appVersion will point to our latest released container image?

I think this would make sense

OK, I think expanding the _helpers labels function to include that label would make sense
WDYT @TylerGillson ?

@TylerGillson
Copy link
Contributor Author

@arbreezy @thschue I'm happy to make whatever changes you guys feel are necessary, but I'll need a little bit more detail please.

We already have app.kubernetes.io/version: "0.2.4" and I proposed app.k8sgpt.image: "ghcr.io/k8sgpt-ai/k8sgpt:v0.2.4". What exactly are you proposing that I add to the default labels?

@thschue
Copy link
Contributor

thschue commented Apr 26, 2023

AlexsJones
AlexsJones previously approved these changes Apr 27, 2023
Copy link
Contributor

@thschue thschue left a comment

Choose a reason for hiding this comment

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

The moved Dockerfile leads to a failing CI build, please move it back to its original location.

Signed-off-by: Tyler Gillson <tyler.gillson@gmail.com>
* remove K8SGPT_BASEURL

* add default model

* add user-specified Deployment annotations

* remove Values.secret.enabled

---------

Signed-off-by: Tyler Gillson <tyler.gillson@gmail.com>
Signed-off-by: Tyler Gillson <tyler.gillson@gmail.com>
Signed-off-by: Tyler Gillson <tyler.gillson@gmail.com>
Signed-off-by: Tyler Gillson <tyler.gillson@gmail.com>
Signed-off-by: Tyler Gillson <tyler.gillson@gmail.com>
thschue
thschue previously approved these changes Apr 27, 2023
Copy link
Contributor

@thschue thschue left a comment

Choose a reason for hiding this comment

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

lgtm

Signed-off-by: Tyler Gillson <tyler.gillson@gmail.com>
charts/k8sgpt/Chart.yaml Outdated Show resolved Hide resolved
@thschue
Copy link
Contributor

thschue commented Apr 27, 2023

@TylerGillson: Made two suggestions, then it should work ...

TylerGillson and others added 2 commits April 27, 2023 11:35
Co-authored-by: Thomas Schuetz <38893055+thschue@users.noreply.github.com>
Signed-off-by: Tyler Gillson <tyler.gillson@gmail.com>
Co-authored-by: Thomas Schuetz <38893055+thschue@users.noreply.github.com>
Signed-off-by: Tyler Gillson <tyler.gillson@gmail.com>
Copy link
Contributor

@thschue thschue 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 for your contribution! LGTM

@thschue thschue merged commit 5af8178 into k8sgpt-ai:main Apr 27, 2023
@TylerGillson TylerGillson deleted the tyler/helm-chart branch April 27, 2023 19:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

feature: Create a Helm Chart for k8sgtp in cluster deployment
5 participants