refactoring helm chart #5353

nithyatsu · 2023-03-28T19:15:36Z

Description

rad install --set command can now be used to set helm values.
This PR is to refactor helm values.yaml structure and provide final documentation for different values that can be set using rad install set.

Issue reference

Fixes: #5343

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

Code compiles correctly
Adds necessary unit tests for change
Adds necessary E2E tests for change
Unit tests passing
Extended the documentation / Created issue for it

github-actions · 2023-03-28T19:24:58Z

For the detailed report, please go to Checks tab, click Build and Test, and then download unit_test_coverage artifact at the bottom of build page.

Your PR branch coverage: 60.4 %
main branch coverage: 60.4 %
diff coverage: 0 %

The coverage result does not include the functional test coverage.

github-actions · 2023-03-28T19:51:42Z

For the detailed report, please go to Checks tab, click Build and Test, and then download unit_test_coverage artifact at the bottom of build page.

Your PR branch coverage: 60.4 %
main branch coverage: 60.4 %
diff coverage: 0 %

The coverage result does not include the functional test coverage.

github-actions · 2023-03-28T20:26:19Z

For the detailed report, please go to Checks tab, click Build and Test, and then download unit_test_coverage artifact at the bottom of build page.

Your PR branch coverage: 60.4 %
main branch coverage: 60.4 %
diff coverage: 0 %

The coverage result does not include the functional test coverage.

github-actions · 2023-03-28T22:10:25Z

For the detailed report, please go to Checks tab, click Build and Test, and then download unit_test_coverage artifact at the bottom of build page.

Your PR branch coverage: 60.4 %
main branch coverage: 60.4 %
diff coverage: 0 %

The coverage result does not include the functional test coverage.

github-actions · 2023-03-28T22:52:52Z

For the detailed report, please go to Checks tab, click Build and Test, and then download unit_test_coverage artifact at the bottom of build page.

Your PR branch coverage: 60.4 %
main branch coverage: 60.4 %
diff coverage: 0 %

The coverage result does not include the functional test coverage.

github-actions · 2023-03-28T23:23:55Z

For the detailed report, please go to Checks tab, click Build and Test, and then download unit_test_coverage artifact at the bottom of build page.

Your PR branch coverage: 60.4 %
main branch coverage: 60.4 %
diff coverage: 0 %

The coverage result does not include the functional test coverage.

youngbupark · 2023-03-29T14:44:33Z

deploy/Chart/charts/rp/templates/rp.yaml

@@ -41,9 +41,9 @@ spec:
          value: 'true'
        - name: SKIP_AUTH
          value: 'true'
-        {{ if .Values.global.rp.publicEndpointOverride}}
+        {{ if .Values.publicEndpointOverride}}


I just want to confirm. Since it is a subchart, it does not need to be .Values.rp.publicEndpointOverride, does it?

oh yes! I have missed it, i will fix it now.

sorry I misread your question. When we do the subcharts, to the subchart, the "subchart:" section in umbrella chart appears just as if it were set in the subchart's local values. file. Therefore we just say Values.publicEndpointOverride. template engine takes care of it..

youngbupark · 2023-03-29T14:45:50Z

pkg/cli/helm/radiusclient.go

+	aws["accessKeyId"] = awsProvider.AccessKeyId
+	aws["secretAccessKey"] = awsProvider.SecretAccessKey


We do not need L323-L324 since there are no code referring to them in helm chart.

youngbupark · 2023-03-29T14:46:28Z

pkg/cli/helm/radiusclient.go

-		"accessKeyId":     awsProvider.AccessKeyId,
-		"secretAccessKey": awsProvider.SecretAccessKey,
-		"region":          awsProvider.TargetRegion,
-	}


Can you please remove addAzureProviderValues too ?

youngbupark

I left minor comments. Please address them before merging :) Also please validate the values in helm chart too.

deploy/Chart/Chart.yaml

deploy/Chart/charts/rp/Chart.yaml

deploy/Chart/values.yaml

ytimocin

LGTM other than a few nits

ytimocin · 2023-03-29T16:07:39Z

deploy/Chart/Chart.yaml

    version: '42.42.42-dev'
-    repository: "file://appcore-rp"
+    repository: "file://rp"


Should this also be radius-rp?

ytimocin · 2023-03-29T16:08:26Z

deploy/Chart/charts/rp/Chart.yaml

@@ -1,5 +1,5 @@
 apiVersion: v2
-name: appcore-rp
+name: rp


radius-rp?

ytimocin · 2023-03-29T16:09:31Z

deploy/Chart/values.yaml

+  #
+  # Configure global.zipkin.url to enable distributed trace
+  # zipkin:
+  #   url: "http://jaeger-collector.radius-monitoring.svc.cluster.local:9411/api/v2/spans"
+  #


Is this also an example?

nit: Could you add "Example" in the comments?

vinayada1

added a nit

github-actions · 2023-03-29T17:07:42Z

For the detailed report, please go to Checks tab, click Build and Test, and then download unit_test_coverage artifact at the bottom of build page.

Your PR branch coverage: 60.6 %
main branch coverage: 60.5 %
diff coverage: .1 %

The coverage result does not include the functional test coverage.

kachawla · 2023-03-29T17:11:35Z

deploy/Chart/charts/radius-rp/Chart.yaml

-name: appcore-rp
-description: Applications.Core Radius service
+name: radius-rp
+description:  Radius service


Suggested change

description: Radius service

description: Radius service

kachawla · 2023-03-29T17:16:54Z

deploy/Chart/values.yaml

+  #   aws: 
+  #     region:"us-east"
+radius-rp:
+  image: radius.azurecr.io/appcore-rp


Do we plan to change the image to radius-rp as well?

not sure.. @youngbupark , should I create a ticket for renaming image?

pkg/cli/helm/radiusclient.go

github-actions · 2023-03-29T18:13:45Z

For the detailed report, please go to Checks tab, click Build and Test, and then download unit_test_coverage artifact at the bottom of build page.

Your PR branch coverage: 60.6 %
main branch coverage: 60.5 %
diff coverage: .1 %

The coverage result does not include the functional test coverage.

github-actions · 2023-03-29T18:25:01Z

For the detailed report, please go to Checks tab, click Build and Test, and then download unit_test_coverage artifact at the bottom of build page.

Your PR branch coverage: 60.6 %
main branch coverage: 60.5 %
diff coverage: .1 %

The coverage result does not include the functional test coverage.

kachawla

Should this client be updated as well to expect updated keys? https://github.com/project-radius/radius/blob/main/pkg/cli/helm/radiusclient.go#L245

nithyatsu added 2 commits March 28, 2023 11:11

refactor service values out of global section

e607f79

wip

5cab345

nithyatsu added 2 commits March 28, 2023 12:33

wip

b3e1e35

wip

06f6893

commit

09edd80

nithyatsu added 2 commits March 28, 2023 14:29

wip

2024e36

wip

f979acf

wip

1477602

nithyatsu added 2 commits March 28, 2023 16:11

nit

9169687

wip

0139b84

nithyatsu marked this pull request as ready for review March 28, 2023 23:50

nithyatsu requested a review from a team as a code owner March 28, 2023 23:50

nithyatsu requested a review from youngbupark March 28, 2023 23:50

youngbupark reviewed Mar 29, 2023

View reviewed changes

youngbupark approved these changes Mar 29, 2023

View reviewed changes

kachawla reviewed Mar 29, 2023

View reviewed changes

deploy/Chart/Chart.yaml Outdated Show resolved Hide resolved

kachawla reviewed Mar 29, 2023

View reviewed changes

deploy/Chart/charts/rp/Chart.yaml Outdated Show resolved Hide resolved

deploy/Chart/values.yaml Show resolved Hide resolved

ytimocin approved these changes Mar 29, 2023

View reviewed changes

nithyatsu added 2 commits March 29, 2023 09:28

wip

5a4635f

remove unused code

db3a3f7

vinayada1 approved these changes Mar 29, 2023

View reviewed changes

kachawla approved these changes Mar 29, 2023

View reviewed changes

nit

5666524

text

f5975aa

nithyatsu merged commit 7617329 into main Mar 29, 2023

nithyatsu deleted the helming branch March 29, 2023 19:23

kachawla reviewed Mar 29, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactoring helm chart #5353

refactoring helm chart #5353

nithyatsu commented Mar 28, 2023

github-actions bot commented Mar 28, 2023

github-actions bot commented Mar 28, 2023

github-actions bot commented Mar 28, 2023

github-actions bot commented Mar 28, 2023

github-actions bot commented Mar 28, 2023

github-actions bot commented Mar 28, 2023

youngbupark Mar 29, 2023

nithyatsu Mar 29, 2023

nithyatsu Mar 29, 2023 •

edited

Loading

youngbupark Mar 29, 2023

youngbupark Mar 29, 2023

youngbupark left a comment

ytimocin left a comment

ytimocin Mar 29, 2023

ytimocin Mar 29, 2023

ytimocin Mar 29, 2023

nithyatsu Mar 29, 2023

vinayada1 Mar 29, 2023

vinayada1 left a comment

github-actions bot commented Mar 29, 2023

kachawla Mar 29, 2023

kachawla Mar 29, 2023

nithyatsu Mar 29, 2023

github-actions bot commented Mar 29, 2023

github-actions bot commented Mar 29, 2023

kachawla left a comment

		aws["accessKeyId"] = awsProvider.AccessKeyId
		aws["secretAccessKey"] = awsProvider.SecretAccessKey

refactoring helm chart #5353

refactoring helm chart #5353

Conversation

nithyatsu commented Mar 28, 2023

Description

Issue reference

Checklist

github-actions bot commented Mar 28, 2023

github-actions bot commented Mar 28, 2023

github-actions bot commented Mar 28, 2023

github-actions bot commented Mar 28, 2023

github-actions bot commented Mar 28, 2023

github-actions bot commented Mar 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nithyatsu Mar 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youngbupark left a comment

Choose a reason for hiding this comment

ytimocin 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

vinayada1 left a comment

Choose a reason for hiding this comment

github-actions bot commented Mar 29, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Mar 29, 2023

github-actions bot commented Mar 29, 2023

kachawla left a comment

Choose a reason for hiding this comment

nithyatsu Mar 29, 2023 •

edited

Loading