feat: Update the helm chart #333

helayoty · 2023-04-14T00:46:33Z

Pull Request

Issue Number: (Link to Github Issue or Azure Dev Ops Task/Story)

Summary

One sentence summary of PR

Changes

List of comprehensive changes

Checklist

Local Tests Passing?
CICD and Pipeline Tests Passing?
Added any new Tests?
Documentation Updates Made?
Are there any API Changes? If yes, please describe below.
This is not a breaking change. If it is, please describe it below.

Are there API Changes?

If yes, what are the expected API Changes? Please link to an API-Comparison
workflow with the API Diff.

Is this a breaking change?

If yes, what workflow does this break?

Anything else?

Other comments, collaborators, etc.

Please follow
GitHub's suggested syntax
to link Pull Requests to Issues via keywords

This PR Closes #<issue_number>

Signed-off-by: Heba Elayoty <[email protected]>

YaSuenag · 2023-04-20T13:38:47Z

I think this PR should happen after deploying 4-release.yml in the plan, but I want you to ask that this example still aims to deploy to AKS? I think it is more useful if it is general, and we can be so because Helm is not only for AKS.

helayoty · 2023-04-26T17:37:45Z

I think this PR should happen after deploying 4-release.yml in the plan, but I want you to ask that this example still aims to deploy to AKS? I think it is more useful if it is general, and we can be so because Helm is not only for AKS.

Nothing related to AKS in this PR.

samples/helm-deploy/values.yaml

YaSuenag · 2023-06-16T09:02:59Z

samples/helm-deploy/templates/deployment.yaml

+          command:
+            - sh
+            - -c
+            - cd /app && sed "s/username/{{ .Values.image.auth.username }}/" -i appsettings.json && sed "s/password/{{ .Values.image.auth.password }}/" -i appsettings.json && dotnet CarbonAware.WebApi.dll


command is needed? WebAPI container has ENTRYPOINT.

For configuration, we should support environment value and/or JSON in deployment.yaml.

This command to inject the username and password into appsettings.json. This file doesn't support env variable afaik.

We can set username and account through environment variable: https://github.com/Green-Software-Foundation/carbon-aware-sdk/blob/dev/docs/quickstart.md#setting-up-the-web-api

Otherwise we should use ConfigMap here.

It would be good to use Secret to store credentials. So it is better approach to have both secrets and configmap (as JSON).

samples/helm-deploy/templates/deployment.yaml

YaSuenag · 2023-06-16T09:05:39Z

samples/helm-deploy/templates/deployment.yaml

+            - cd /app && sed "s/username/{{ .Values.image.auth.username }}/" -i appsettings.json && sed "s/password/{{ .Values.image.auth.password }}/" -i appsettings.json && dotnet CarbonAware.WebApi.dll
+          ports:
+            - name: api-server-port
+              containerPort: 7031


Should it be 80?

not necessary

What is 7031 for? I believe WebAPI container will expose 80, and it should be set as containerPort.

Signed-off-by: Heba Elayoty <[email protected]>

YaSuenag · 2023-06-17T00:50:07Z

samples/helm-deploy/templates/deployment.yaml

+          command:
+            - sh
+            - -c
+            - cd /app && sed "s/username/{{ .Values.image.auth.username }}/" -i appsettings.json && sed "s/password/{{ .Values.image.auth.password }}/" -i appsettings.json && dotnet CarbonAware.WebApi.dll


We can set username and account through environment variable: https://github.com/Green-Software-Foundation/carbon-aware-sdk/blob/dev/docs/quickstart.md#setting-up-the-web-api

Otherwise we should use ConfigMap here.

YaSuenag · 2023-06-17T01:03:10Z

samples/helm-deploy/templates/service.yaml

+    - port: {{ .Values.service.port }}
+      targetPort: api-server-port


I guess you want to specify the port what the user want to expose in values.yaml, and also it should be so. Thus api-server-port should be set to port, and also .Values.service,.port should be set to targetPort?

YaSuenag · 2023-06-17T01:06:19Z

samples/helm-deploy/values.yaml

+  auth:
+    username: username
+    password: password


We should consider other parameters (e.g. data source) if we can set them via environment variables and/or ConfigMap.

YaSuenag · 2023-06-17T01:06:45Z

samples/helm-deploy/templates/serviceaccount.yaml

This YAML is no longer needed.

YaSuenag · 2023-06-17T01:12:01Z

samples/helm-deploy/templates/deployment.yaml

+kind: Deployment
+metadata:
+  name: {{ include "carbon-aware-sdk.fullname" . }}
+  namespace: {{ .Values.namespace }}


We should use .Release.namespace rather than the value from values.yaml. https://helm.sh/docs/chart_template_guide/builtin_objects/

YaSuenag · 2023-06-17T01:15:04Z

samples/helm-deploy/README.md

+## Push an image to the Docker registry
+
+The following steps illustrates how to push a webservice image in order to be
+deployed in Kubernetes clsuter.
+
+1. Build an image using the following
+   [Dockerfile](.../../../../src/CarbonAware.WebApi/src/Dockerfile)
+
+   ```sh
+   cd <Dockerfile path>
+   docker build -t <registry>/myapp:v1 .
+   ```
+
+1. Login into docker using the username and password credentials that are needed in
+   order to push.
+
+   ```sh
+   docker login 
+   ```
+
+1. Push to Docker registry 
+
+   ```sh
+   docker push <registry>/myapp:v1
+   ```


This section is not needed because default values.yaml refers container image from GitHub Container Registry.

YaSuenag · 2023-06-17T01:16:13Z

samples/helm-deploy/README.md

+Set the `nameOverride` and `fullNameOverride` fields to make it easier to
+reference your helm chart, and ensure they are not the same.
+
+In the `serviceAccount` section, ensure that


serviceAccount is no longer needed.

YaSuenag · 2023-06-17T01:18:56Z

samples/helm-deploy/README.md

+In the `monitorConfig` section, you will need to adjust the `liveness` and
+`readiness` that the helm chart will ping to ensure the sdk is ready. As a
+default, you should set the path to `/health`.


Where is monitorConfig? Liveness / Startup probe should be set into the deployment yaml.

YaSuenag · 2023-06-17T01:19:31Z

samples/helm-deploy/README.md

+In `Chart.yaml`, we won't need to make any changes but make note of the chart
+`name`, as you will need to reference it in commands later on.
+
+### Values.yaml


We should guide account settings for data source.

YaSuenag · 2023-06-17T01:20:59Z

samples/helm-deploy/README.md

+To install your helm chart, you should run
+
+```bash
+helm install carbon-aware ./samples/helm-deploy/


Maybe it is better to good with -n <namespace> and --create-namespace. IMHO carbon-aware-sdk is good for namespace.

Willmish · 2023-07-11T07:27:57Z

Thanks for contiuing to work on this!
@helayoty can you look at @YaSuenag comments + review?

vaughanknight · 2023-07-25T07:50:46Z

@YaSuenag - are you able to fork the source branch helayoty:helm-update and add your changes, and do a separate PR for those updates please?

YaSuenag

I added some changes as a suggestion. Please apply if you are ok, then I will try to send PR for your branch because some files will be added/removed.

YaSuenag · 2023-07-28T01:59:56Z

samples/helm-deploy/README.md

+## Push an image to the Docker registry
+
+The following steps illustrates how to push a webservice image in order to be
+deployed in Kubernetes clsuter.
+
+1. Build an image using the following
+   [Dockerfile](.../../../../src/CarbonAware.WebApi/src/Dockerfile)
+
+   ```sh
+   cd <Dockerfile path>
+   docker build -t <registry>/myapp:v1 .
+   ```
+
+1. Login into docker using the username and password credentials that are needed in
+   order to push.
+
+   ```sh
+   docker login 
+   ```
+
+1. Push to Docker registry 
+
+   ```sh
+   docker push <registry>/myapp:v1
+   ```
+


Suggested change

## Push an image to the Docker registry

The following steps illustrates how to push a webservice image in order to be

deployed in Kubernetes clsuter.

1. Build an image using the following

[Dockerfile](.../../../../src/CarbonAware.WebApi/src/Dockerfile)

```sh

cd <Dockerfile path>

docker build -t <registry>/myapp:v1 .

```

1. Login into docker using the username and password credentials that are needed in

order to push.

```sh

docker login

```

1. Push to Docker registry

```sh

docker push <registry>/myapp:v1

```

YaSuenag · 2023-07-28T02:01:10Z

samples/helm-deploy/README.md

+In the `serviceAccount` section, ensure that
+
+- The `name` field is set to the name of the helm chart from `Chart.yaml`.
+


Suggested change

In the `serviceAccount` section, ensure that

- The `name` field is set to the name of the helm chart from `Chart.yaml`.

YaSuenag · 2023-07-28T02:02:27Z

samples/helm-deploy/README.md

+To install your helm chart, you should run
+
+```bash
+helm install carbon-aware ./samples/helm-deploy/


Suggested change

helm install carbon-aware ./samples/helm-deploy/

helm install -n carbon-aware-sdk --create-namespace carbon-aware ./samples/helm-deploy/

YaSuenag · 2023-07-28T02:03:15Z

samples/helm-deploy/templates/deployment.yaml

+kind: Deployment
+metadata:
+  name: {{ include "carbon-aware-sdk.fullname" . }}
+  namespace: {{ .Values.namespace }}


Suggested change

namespace: {{ .Values.namespace }}

namespace: {{ .Release.namespace }}

YaSuenag · 2023-07-28T02:04:16Z

samples/helm-deploy/templates/deployment.yaml

+            - cd /app && sed "s/username/{{ .Values.image.auth.username }}/" -i appsettings.json && sed "s/password/{{ .Values.image.auth.password }}/" -i appsettings.json && dotnet CarbonAware.WebApi.dll
+          ports:
+            - name: api-server-port
+              containerPort: 7031


Suggested change

containerPort: 7031

containerPort: 80

YaSuenag · 2023-07-28T02:14:16Z

samples/helm-deploy/templates/deployment.yaml

+          ports:
+            - name: api-server-port
+              containerPort: 7031
+


Suggested change

startupProbe:

httpGet:

path: /health

port: 80

failureThreshold: 10

periodSeconds: 5

livenessProbe:

httpGet:

path: /health

port: 80

periodSeconds: 30

YaSuenag · 2023-07-28T02:14:34Z

samples/helm-deploy/templates/service.yaml

+kind: Service
+metadata:
+  name: carbon-aware-sdk
+  namespace: {{ .Values.namespace }}


Suggested change

namespace: {{ .Values.namespace }}

namespace: {{ .Release.namespace }}

YaSuenag · 2023-08-05T14:50:09Z

I think Helm chart for CASDK should be official artifact, not an example, and also it should be published for the user can install it via Helm. So I'd like to propose the chart and GHA workflow to publish it as a container. How about it?

dev...YaSuenag:carbon-aware-sdk:dev-helm

This branch would publish Helm chart to GitHub container registry, so you can pull it as a chart via helm command when you use Helm v3.8.0 or later: https://helm.sh/docs/topics/registries/

You can set appsettings.json as a Secret resource, and deploy CASDK.

values.yaml

image:
  tag: v1.1.0

appsettings: |-
  {
    "DataSources": {
      "EmissionsDataSource": "WattTime",
      "ForecastDataSource": "WattTime",
      "Configurations": {
        "WattTime": {
          "Type": "WattTime",
          "Username": "username",
          "Password": "password",
          "BaseURL": "https://api2.watttime.org/v2/"
        }
      }
    }
  }

Install command

helm install casdk -n carbon-aware-sdk --create-namespace oci://ghcr.io/yasuenag/charts/carbon-aware-sdk --values casdk-values.yaml

@vaughanknight @Willmish @danuw Can you agree with publishing Helm chart container from GitHub container registry of CASDK? I believe it can help the user who want to use the SDK on Kubernetes.

My Helm chart repo is here: https://github.com/YaSuenag/carbon-aware-sdk/pkgs/container/charts%2Fcarbon-aware-sdk

danuw · 2023-08-05T22:45:21Z

Sounds great!

Would love to test this too if we can find time to connect.

Fyi I cant make it Tuesday morning so I will connect directly

Thank you yasumasa

YaSuenag · 2023-08-06T01:30:00Z

@helayoty Can I take over this PR? I'll create new one from my side if you are ok.

YaSuenag · 2023-08-08T01:04:02Z

Thanks @helayoty for giving +1 emoji! I've created new PR. It's appreciated if you can review.

#381

And also this PR should be closed, I think.

Update the helm chart

df4a3c6

Signed-off-by: Heba Elayoty <[email protected]>

helayoty requested a review from vaughanknight as a code owner April 14, 2023 00:46

Willmish mentioned this pull request Apr 25, 2023

🎯2023-04-25 Carbon Aware SDK Weekly Meeting Agenda #337

Closed

36 tasks

Willmish mentioned this pull request Jun 13, 2023

🎯 2023-06-13 Carbon Aware SDK Weekly Meeting Agenda #363

Closed

18 tasks

YaSuenag suggested changes Jun 16, 2023

View reviewed changes

helayoty force-pushed the helm-update branch 2 times, most recently from be044b7 to df4a3c6 Compare June 16, 2023 19:52

helayoty added 2 commits June 16, 2023 19:56

Code review comments

2d02019

Signed-off-by: Heba Elayoty <[email protected]>

Update readme

aa0936e

Signed-off-by: Heba Elayoty <[email protected]>

helayoty requested a review from YaSuenag June 16, 2023 20:14

YaSuenag suggested changes Jun 17, 2023

View reviewed changes

Willmish mentioned this pull request Jul 11, 2023

🎯 2023-07-11 Carbon Aware SDK Weekly Meeting Agenda #369

Closed

17 tasks

YaSuenag reviewed Jul 28, 2023

View reviewed changes

YaSuenag mentioned this pull request Aug 8, 2023

Add Helm chart and workflow #381

Merged

6 tasks

helayoty closed this Aug 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Update the helm chart #333

feat: Update the helm chart #333

helayoty commented Apr 14, 2023

YaSuenag commented Apr 20, 2023

helayoty commented Apr 26, 2023

YaSuenag Jun 16, 2023

helayoty Jun 16, 2023

YaSuenag Jun 17, 2023

YaSuenag Jun 17, 2023

YaSuenag Jun 16, 2023

helayoty Jun 16, 2023

YaSuenag Jun 17, 2023 •

edited

Loading

YaSuenag Jun 17, 2023

YaSuenag Jun 17, 2023

YaSuenag Jun 17, 2023

YaSuenag Jun 17, 2023

YaSuenag Jun 17, 2023

YaSuenag Jun 17, 2023

YaSuenag Jun 17, 2023

YaSuenag Jun 17, 2023

YaSuenag Jun 17, 2023

YaSuenag Jun 17, 2023

Willmish commented Jul 11, 2023

vaughanknight commented Jul 25, 2023

YaSuenag left a comment

YaSuenag Jul 28, 2023

YaSuenag Jul 28, 2023

YaSuenag Jul 28, 2023

YaSuenag Jul 28, 2023

YaSuenag Jul 28, 2023

YaSuenag Jul 28, 2023

YaSuenag Jul 28, 2023

YaSuenag commented Aug 5, 2023

danuw commented Aug 5, 2023 •

edited

Loading

YaSuenag commented Aug 6, 2023

YaSuenag commented Aug 8, 2023

		- port: {{ .Values.service.port }}
		targetPort: api-server-port

		In the `serviceAccount` section, ensure that

		- The `name` field is set to the name of the helm chart from `Chart.yaml`.

	helm install carbon-aware ./samples/helm-deploy/
	helm install -n carbon-aware-sdk --create-namespace carbon-aware ./samples/helm-deploy/

	namespace: {{ .Values.namespace }}
	namespace: {{ .Release.namespace }}

+          startupProbe:
+            httpGet:
+              path: /health
+              port: 80
+            failureThreshold: 10
+            periodSeconds: 5
+          livenessProbe:
+            httpGet:
+              path: /health
+              port: 80
+            periodSeconds: 30

feat: Update the helm chart #333

feat: Update the helm chart #333

Conversation

helayoty commented Apr 14, 2023

Pull Request

Summary

Changes

Checklist

Are there API Changes?

Is this a breaking change?

Anything else?

YaSuenag commented Apr 20, 2023

helayoty commented Apr 26, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YaSuenag Jun 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Willmish commented Jul 11, 2023

vaughanknight commented Jul 25, 2023

YaSuenag left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YaSuenag commented Aug 5, 2023

danuw commented Aug 5, 2023 • edited Loading

YaSuenag commented Aug 6, 2023

YaSuenag commented Aug 8, 2023

YaSuenag Jun 17, 2023 •

edited

Loading

danuw commented Aug 5, 2023 •

edited

Loading