Add support for template kind in Environment Recipe schema #5575

kachawla · 2023-05-18T17:07:31Z

Description

With the addition of Terraform (and other formats in the future), we need a signal from the user to be able to differentiate what type does the recipe belong to. Adding a new field TemplateKind in Env Recipe properties.

Initial review of this field was done as a part of, open to feedback if there are questions/inputs on the naming choice here.
https://microsoft.sharepoint.com/:w:/r/teams/radiuscoreteam/_layouts/15/Doc.aspx?sourcedoc=%7B0F69ADC3-A8F8-4EDC-8215-841C00B68897%7D&file=2023-04%20Radius%20Tech%20Review%20-%20Recipes%20Grab-bag.docx&nav=eyJjIjo2NzAyMTY5MTN9&action=default&mobileredirect=true&share=IQHDrWkP-KjcToIVhBwAtoiXAVD5zjtUgyIJQusNjp-HGko&cid=dae1bb79-1ea9-44ea-8a47-dff69d5091c0

Issue reference

Fixes Support multiple template types user story under https://github.com/project-radius/radius/issues/4446.

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 -- Will update existing tests after bicep update is merged
Unit tests passing
Extended the documentation / Created issue for it -- Created issue

github-actions · 2023-05-18T17:07:45Z

You are editing files which require a docs update. Please ensure you've made the appropriate changes to the docs and submitted a PR.

Swagger changes require reference edge docs to be updated here
Make sure to create a branch and submit a PR into the edge branch instead of the default latest branch, as the swagger changes will not be available until the next release.

For more information on contributing to docs please visit https://docs.radapp.dev/contributing/docs/.

github-actions · 2023-05-18T17:10:41Z

Radius functional test overview

🔍 Go to test action run

Name	Value
Repository	project-radius/radius
Commit ref	`6142e4f`
Unique ID	e57fe873c0
Image tag	pr-e57fe873c0

Click here to see the list of tools in the current test run

KinD: v0.18.0
Dapr: 1.10.0
Azure KeyVault CSI driver: 1.4.2
Azure Workload identity webhook: 1.0.0
recipe location radiusdev.azurecr.io/test/functional/corerp/recipes/<name>:pr-e57fe873c0
appcore-rp test image location: radiusdev.azurecr.io/appcore-rp:pr-e57fe873c0
ucp test image location: radiusdev.azurecr.io/ucpd:pr-e57fe873c0

Test Status

⌛ Building Radius and pushing container images for functional tests...
✅ Container images build succeeded
⌛ Publishing Bicep Recipes for functional tests...
✅ Recipe publishing succeeded
⌛ Starting samples functional tests...
⌛ Starting corerp functional tests...
⌛ Starting ucp functional tests...
❌ Test tool installation for samples failed. Please check the logs for more details
❌ Failed to install Radius for samples functional test. Please check the logs for more details
❌ samples functional test failed. Please check the logs for more details

kachawla · 2023-05-18T17:12:57Z

...tions/resource-manager/Applications.Core/preview/2022-03-15-privatepreview/environments.json

@@ -479,6 +480,10 @@
    "EnvironmentRecipeProperties": {
      "description": "Properties of a Recipe linked to an Environment.",
      "properties": {
+        "templateKind": {


Planning to mark it as required in a separate PR after bicep is updated. @AaronCrawfis @rynowak, does it sound good if we add this as a required field in the upcoming release? (users will have to update their existing templates)

Not adding an enum just yet, since we do want to use this field to test terraform, without officially exposing it to users.

Yeah either way users will need to update their templates, so I'm good with making this optional for now and then changing it to required in a future PR

Perfect, thanks! Will also include templateKind in Recipe commands after this PR and bicep is merged.

Yes. Agree that this needs to become required.

Here's the PR - #5580

pkg/corerp/frontend/controller/environments/createorupdateenvironment.go

github-actions · 2023-05-18T17:18:03Z

Test Results

2 703 tests +1 2 696 ✔️ +1 1m 55s ⏱️ ±0s
  242 suites ±0       7 💤 ±0
      1 files ±0       0 ❌ ±0

Results for commit 2fb419b. ± Comparison against base commit 5824e84.

This pull request removes 2 and adds 3 tests. Note that renamed tests count towards both.

github.com/project-radius/radius/pkg/armrpc/frontend/controller ‑ TestValidateEtag_IfMatch/7f639983-a4d2-4f28-96ab-cfc6c8ecc61b
github.com/project-radius/radius/pkg/armrpc/frontend/controller ‑ TestValidateEtag_IfMatch/7f639983-a4d2-4f28-96ab-cfc6c8ecc61b#01

github.com/project-radius/radius/pkg/armrpc/frontend/controller ‑ TestValidateEtag_IfMatch/c05ab2b2-5fd0-46f5-9c66-bd611af213ec
github.com/project-radius/radius/pkg/armrpc/frontend/controller ‑ TestValidateEtag_IfMatch/c05ab2b2-5fd0-46f5-9c66-bd611af213ec#01
github.com/project-radius/radius/pkg/corerp/api/v20220315privatepreview ‑ TestConvertVersionedToDataModel/environmentresource-invalid-templatekind.json

♻️ This comment has been updated with latest results.

github-actions · 2023-05-18T17:18:15Z

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: 63.9 %
main branch coverage: 63.9 %
diff coverage: 0 %

The coverage result does not include the functional test coverage.

...tions/resource-manager/Applications.Core/preview/2022-03-15-privatepreview/environments.json

github-actions · 2023-05-18T17:23:24Z

You are editing files which require a docs update. Please ensure you've made the appropriate changes to the docs and submitted a PR.

Swagger changes require reference edge docs to be updated here
Make sure to create a branch and submit a PR into the edge branch instead of the default latest branch, as the swagger changes will not be available until the next release.

For more information on contributing to docs please visit https://docs.radapp.dev/contributing/docs/.

github-actions · 2023-05-18T17:26:28Z

Radius functional test overview

🔍 Go to test action run

Name	Value
Repository	project-radius/radius
Commit ref	`0ce3b99`
Unique ID	26998b49f0
Image tag	pr-26998b49f0

Click here to see the list of tools in the current test run

KinD: v0.18.0
Dapr: 1.10.0
Azure KeyVault CSI driver: 1.4.2
Azure Workload identity webhook: 1.0.0
recipe location radiusdev.azurecr.io/test/functional/corerp/recipes/<name>:pr-26998b49f0
appcore-rp test image location: radiusdev.azurecr.io/appcore-rp:pr-26998b49f0
ucp test image location: radiusdev.azurecr.io/ucpd:pr-26998b49f0

Test Status

⌛ Building Radius and pushing container images for functional tests...
✅ Container images build succeeded
⌛ Publishing Bicep Recipes for functional tests...
✅ Recipe publishing succeeded
⌛ Starting corerp functional tests...
⌛ Starting samples functional tests...
❌ Test tool installation for samples failed. Please check the logs for more details
❌ Failed to install Radius for samples functional test. Please check the logs for more details
❌ samples functional test failed. Please check the logs for more details

github-actions · 2023-05-18T17:33: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: 63.9 %
main branch coverage: 63.9 %
diff coverage: 0 %

The coverage result does not include the functional test coverage.

github-actions · 2023-05-18T19:53:53Z

You are editing files which require a docs update. Please ensure you've made the appropriate changes to the docs and submitted a PR.

Swagger changes require reference edge docs to be updated here
Make sure to create a branch and submit a PR into the edge branch instead of the default latest branch, as the swagger changes will not be available until the next release.

For more information on contributing to docs please visit https://docs.radapp.dev/contributing/docs/.

github-actions · 2023-05-18T19:54:34Z

Radius functional test overview

🔍 Go to test action run

Name	Value
Repository	project-radius/radius
Commit ref	`2ef9ec2`
Unique ID	3e575eb2af
Image tag	pr-3e575eb2af

Click here to see the list of tools in the current test run

KinD: v0.18.0
Dapr: 1.10.0
Azure KeyVault CSI driver: 1.4.2
Azure Workload identity webhook: 1.0.0
recipe location radiusdev.azurecr.io/test/functional/corerp/recipes/<name>:pr-3e575eb2af
appcore-rp test image location: radiusdev.azurecr.io/appcore-rp:pr-3e575eb2af
ucp test image location: radiusdev.azurecr.io/ucpd:pr-3e575eb2af

Test Status

⌛ Building Radius and pushing container images for functional tests...
✅ Container images build succeeded
⌛ Publishing Bicep Recipes for functional tests...
❌ Test recipe publishing failed

github-actions · 2023-05-18T20:04:07Z

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: 63.9 %
main branch coverage: 63.9 %
diff coverage: 0 %

The coverage result does not include the functional test coverage.

vishwahiremat · 2023-05-18T20:03:44Z

...tions/resource-manager/Applications.Core/preview/2022-03-15-privatepreview/environments.json

+        "templateKind": {
+          "description": "Format of the template provided by the recipe. Allowed values: bicep",
+          "type": "string"
+        },


is it better to make it an enum?

Yes eventually, added some details on why not in this comment here #5575 (comment).

Mainly keeping it open ended right so that we can test Terraform before it is fully integrated and available for users.

pkg/recipes/types.go

ytimocin

LGTM (Do we need to add more tests for TemplateKindTerraform?)

pkg/corerp/api/v20220315privatepreview/environment_conversion_test.go

pkg/corerp/api/v20220315privatepreview/environmentrecipeproperties_conversion_test.go

pkg/corerp/frontend/controller/environments/createorupdateenvironment.go

ytimocin · 2023-05-18T20:13:30Z

pkg/corerp/frontend/controller/environments/createorupdateenvironment_test.go

@@ -403,11 +404,13 @@ var mockgetDevRecipes = func(ctx context.Context) (map[string]map[string]datamod
 	recipes := map[string]map[string]datamodel.EnvironmentRecipeProperties{
 		linkrp.RedisCachesResourceType: {
 			"redis-kubernetes": {
+				TemplateKind: recipes.TemplateKindBicep,


Should we add a test for TemplateKindTerraform?

for dev recipes?

ytimocin · 2023-05-18T20:14:31Z

...e-manager/Applications.Core/preview/2022-03-15-privatepreview/examples/EnvironmentsList.json

                    "templatePath": "br:sampleregistry.azureacr.io/radius/recipes/mongo"
                  }
                },
                "Applications.Link/redisCaches":{
                  "redis-recipe": {
-                    "linkType": "Applications.Link/redisCaches",


vishwahiremat · 2023-05-18T20:19:28Z

Are we adding CLI changes in a different PR?

shalabhms · 2023-05-18T20:37:34Z

@kachawla , you might need to remove ADO issue reference and just reference the main GH issue and call out the changes as part of the feature since we do not have GH issues for tasks. Also we need to remove reference to internal SharePoint team docs. @AaronCrawfis , please confirm on this internal references for GH issue and guidance if any.

shalabhms · 2023-05-18T21:14:30Z

pkg/recipes/configloader/environment.go

@@ -112,7 +112,7 @@ func getRecipeDefinition(environment *v20220315privatepreview.EnvironmentResourc
 	}

 	return &recipes.Definition{
-		Driver:       recipes.DriverBicep,
+		Driver:       *found.TemplateKind,


nit. This is pre-existing and not related to this PR but seems like this is actually kind of driver than "driver" itself. This would require changes to references. cc @vishwahiremat

Suggested change

Driver: *found.TemplateKind,

DriverKind: *found.TemplateKind,

I'm not sure I follow. Can you elaborate a bit more? If you are interested in how it is integrated and consumed for multiple drivers, you can look at this PR #5533 (this PR is not ready for review, so I'm not expecting feedback on syntax and structure there).

On this is actually kind of driver than "driver" itself - current recipes.DriverBicep is not Driver as well, it is a string which defines the type the driver belongs, so we aren't changing anything fundamental here.

shalabhms

/lgtm - nothing blocking . We need to close on the decision on TF dev Recipes which I think that is not blocking this PR changes and can be addressed/revisited based on the decision. Thanks !

shalabhms · 2023-05-18T21:51:38Z

@kachawla , you might need to remove ADO issue reference and just reference the main GH issue and call out the changes as part of the feature since we do not have GH issues for tasks. Also we need to remove reference to internal SharePoint team docs. @AaronCrawfis , please confirm on this internal references for GH issue and guidance if any.

I have updated and linked to the main GH issue now and removed the ADO reference.

kachawla · 2023-05-18T21:55:13Z

@kachawla , you might need to remove ADO issue reference and just reference the main GH issue and call out the changes as part of the feature since we do not have GH issues for tasks. Also we need to remove reference to internal SharePoint team docs. @AaronCrawfis , please confirm on this internal references for GH issue and guidance if any.

I have updated and linked to the main GH issue now and removed the ADO reference.

I'm curious why we shouldn't reference ADO issues? Whenever I see a PR linked to parent GH issue with multiple sub tasks, it becomes difficult to figure out which user story and which task to look at to get specific details.

github-actions · 2023-05-18T22:03:40Z

You are editing files which require a docs update. Please ensure you've made the appropriate changes to the docs and submitted a PR.

Swagger changes require reference edge docs to be updated here
Make sure to create a branch and submit a PR into the edge branch instead of the default latest branch, as the swagger changes will not be available until the next release.

For more information on contributing to docs please visit https://docs.radapp.dev/contributing/docs/.

kachawla · 2023-05-18T22:06:37Z

/lgtm - nothing blocking . We need to close on the decision on TF dev Recipes which I think that is not blocking this PR changes and can be addressed/revisited based on the decision. Thanks !

I'd expect that to covered as a part of #5528 / #5526, since the code updated here is getting removed in one of these PRs.

kachawla · 2023-05-18T22:08:47Z

Are we adding CLI changes in a different PR?

Yeah added a quick note here earlier #5575 (comment), planning to do that after bicep and RP updates are merged, to keep the PRs scoped to smaller set of changes.

github-actions · 2023-05-18T22:14:11Z

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: 63.9 %
main branch coverage: 63.9 %
diff coverage: 0 %

The coverage result does not include the functional test coverage.

github-actions · 2023-05-18T22:20:23Z

⌛ Building Radius and pushing container images for functional tests...

✅ Container images build succeeded
⌛ Publishing Bicep Recipes for functional tests...
✅ Recipe publishing succeeded
⌛ Starting corerp functional tests...
⌛ Starting ucp functional tests...
⌛ Starting samples functional tests...
❌ Test tool installation for corerp failed. Please check the logs for more details
❌ Failed to install Radius for corerp functional test. Please check the logs for more details
❌ corerp functional test failed. Please check the logs for more details
⌛ Starting corerp functional tests...
⌛ Starting samples functional tests...
⌛ Starting ucp functional tests...
❌ Test tool installation for ucp failed. Please check the logs for more details
❌ Failed to install Radius for ucp functional test. Please check the logs for more details
❌ ucp functional test failed. Please check the logs for more details
⌛ Starting ucp functional tests...
⌛ Starting samples functional tests...
⌛ Starting corerp functional tests...
❌ Test tool installation for samples failed. Please check the logs for more details
❌ Failed to install Radius for samples functional test. Please check the logs for more details
❌ samples functional test failed. Please check the logs for more details
⌛ Starting corerp functional tests...
⌛ Starting ucp functional tests...
⌛ Starting samples functional tests...
❌ Test tool installation for corerp failed. Please check the logs for more details
❌ Failed to install Radius for corerp functional test. Please check the logs for more details
❌ Test tool installation for samples failed. Please check the logs for more details
❌ corerp functional test failed. Please check the logs for more details
❌ Failed to install Radius for samples functional test. Please check the logs for more details
❌ samples functional test failed. Please check the logs for more details
⌛ Starting corerp functional tests...
⌛ Starting ucp functional tests...
⌛ Starting samples functional tests...
❌ Test tool installation for corerp failed. Please check the logs for more details
❌ Failed to install Radius for corerp functional test. Please check the logs for more details
❌ corerp functional test failed. Please check the logs for more details
❌ Test tool installation for samples failed. Please check the logs for more details
❌ Failed to install Radius for samples functional test. Please check the logs for more details
⌛ Starting ucp functional tests...
⌛ Starting corerp functional tests...
⌛ Starting samples functional tests...
❌ Test tool installation for ucp failed. Please check the logs for more details
❌ Failed to install Radius for ucp functional test. Please check the logs for more details
❌ ucp functional test failed. Please check the logs for more details
❌ Test tool installation for samples failed. Please check the logs for more details
❌ Failed to install Radius for samples functional test. Please check the logs for more details
❌ samples functional test failed. Please check the logs for more details
⌛ Starting corerp functional tests...
⌛ Starting ucp functional tests...
⌛ Starting samples functional tests...
✅ ucp functional tests succeeded
✅ samples functional tests succeeded
❌ corerp functional test failed. Please check the logs for more details
⌛ Starting corerp functional tests...
✅ corerp functional tests succeeded

rynowak · 2023-05-20T19:22:08Z

pkg/corerp/api/v20220315privatepreview/util.go

@@ -124,3 +125,7 @@ func isValidLinkType(link string) bool {
 	}
 	return slices.Contains(linkTypes, link)
 }
+
+func isValidTemplateKind(link string) bool {
+	return slices.Contains(recipes.SupportedTemplateKind, link)


rynowak

# Description Added a new property for recipe templateKind in here #5575. Switching the property from optional to required and updating functional test templates. ## Issue reference Fixes Support multiple template types user story under https://github.com/project-radius/radius/issues/4446. ## Checklist Please make sure you've completed the relevant tasks for this PR, out of the following list: * [x] Code compiles correctly * [x] Adds necessary unit tests for change * [x] Adds necessary E2E tests for change * [x] Unit tests passing * [x] Extended the documentation / Created issue for it -- Created issue --------- Co-authored-by: karishma-chawla <[email protected]>

kachawla added 5 commits May 17, 2023 18:52

Add templateKind to environment recipe properties

c1a4efa

Update datamodel, converter, tests/testdata

a14ec54

Add validation for supported template formats

e93b1da

rebase

eb9c568

Make templateKind optional till bicep updates are merged

6142e4f

kachawla requested review from rynowak and AaronCrawfis May 18, 2023 17:07

kachawla requested a review from a team as a code owner May 18, 2023 17:07

kachawla commented May 18, 2023

View reviewed changes

pkg/corerp/frontend/controller/environments/createorupdateenvironment.go Show resolved Hide resolved

kachawla requested a review from vishwahiremat May 18, 2023 17:17

kachawla mentioned this pull request May 18, 2023

[Draft] Implementing Terraform driver #5533

Closed

5 tasks

AaronCrawfis reviewed May 18, 2023

View reviewed changes

...tions/resource-manager/Applications.Core/preview/2022-03-15-privatepreview/environments.json Show resolved Hide resolved

Update unit test

2ef9ec2

kachawla force-pushed the kachawla/recipe-templatekind branch from 0ce3b99 to 2ef9ec2 Compare May 18, 2023 19:53

vishwahiremat reviewed May 18, 2023

View reviewed changes

ytimocin approved these changes May 18, 2023

View reviewed changes

shalabhms reviewed May 18, 2023

View reviewed changes

shalabhms approved these changes May 18, 2023

View reviewed changes

Remove unused constant

2fb419b

This comment has been minimized.

Sign in to view

vishwahiremat approved these changes May 18, 2023

View reviewed changes

kachawla merged commit 1a2c37d into main May 19, 2023

kachawla deleted the kachawla/recipe-templatekind branch May 19, 2023 18:00

kachawla mentioned this pull request May 19, 2023

Make TemplateKind required #5580

Merged

5 tasks

rynowak reviewed May 20, 2023

View reviewed changes

	Driver: *found.TemplateKind,
	DriverKind: *found.TemplateKind,

Add support for template kind in Environment Recipe schema #5575

Add support for template kind in Environment Recipe schema #5575

Conversation

kachawla commented May 18, 2023 • edited Loading

Description

Issue reference

Checklist

github-actions bot commented May 18, 2023

github-actions bot commented May 18, 2023 • edited Loading

Radius functional test overview

Test Status

kachawla May 18, 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

github-actions bot commented May 18, 2023 • edited Loading

Test Results

github-actions bot commented May 18, 2023

github-actions bot commented May 18, 2023

github-actions bot commented May 18, 2023 • edited Loading

Radius functional test overview

Test Status

github-actions bot commented May 18, 2023

github-actions bot commented May 18, 2023

github-actions bot commented May 18, 2023 • edited Loading

Radius functional test overview

Test Status

github-actions bot commented May 18, 2023

Choose a reason for hiding this 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

vishwahiremat commented May 18, 2023

shalabhms commented May 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shalabhms left a comment

Choose a reason for hiding this comment

shalabhms commented May 18, 2023

kachawla commented May 18, 2023 • edited Loading

github-actions bot commented May 18, 2023

This comment has been minimized.

kachawla commented May 18, 2023 • edited Loading

kachawla commented May 18, 2023

github-actions bot commented May 18, 2023

github-actions bot commented May 18, 2023 • edited Loading

Choose a reason for hiding this comment

rynowak left a comment

Choose a reason for hiding this comment

kachawla commented May 18, 2023 •

edited

Loading

github-actions bot commented May 18, 2023 •

edited

Loading

kachawla May 18, 2023 •

edited

Loading

github-actions bot commented May 18, 2023 •

edited

Loading

github-actions bot commented May 18, 2023 •

edited

Loading

github-actions bot commented May 18, 2023 •

edited

Loading

shalabhms commented May 18, 2023 •

edited

Loading

kachawla commented May 18, 2023 •

edited

Loading

kachawla commented May 18, 2023 •

edited

Loading

github-actions bot commented May 18, 2023 •

edited

Loading