remove validation of resource types against the static list in recipes codebase #8391
Conversation
nithyatsu
temporarily deployed
to
publish-bicep
GitHub Actions
Inactive
nithyatsu
temporarily deployed
to
publish-bicep
GitHub Actions
Inactive
nithyatsu
temporarily deployed
to
publish-bicep
GitHub Actions
Inactive
nithyatsu
temporarily deployed
to
publish-bicep
GitHub Actions
Inactive
nithyatsu
changed the title
nithyatsu
force-pushed
the
cleanup_recipes
branch
from
ddac45e to
97aba5c
Compare
nithyatsu
temporarily deployed
to
publish-bicep
GitHub Actions
Inactive
nithyatsu
temporarily deployed
to
publish-bicep
GitHub Actions
Inactive
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8391 +/- ##
==========================================
- Coverage 59.94% 59.94% -0.01%
==========================================
Files 596 595 -1
Lines 40432 40435 +3
==========================================
+ Hits 24238 24239 +1
+ Misses 14367 14364 -3
- Partials 1827 1832 +5 ☔ View full report in Codecov by Sentry. |
| @@ -60,16 +65,66 @@ func getDataModel(id resources.ID) (v1.DataModelInterface, error) { | |||
| } | |||
| } | |||
|
|
|||
| func isPortableResource(resourceTypeResourceObj *database.Object) (bool, error) { | |||
There was a problem hiding this comment.
Couple of callouts/questions about this approach -
- Will this work for built in portable types in their current state before they are migrated to the UDT schema (with capabilites/supportsRecipe property)?
- Is the output of this function only used for currently implemented built in portable resources or for UDT resources as well?
- If this is expected to work for UDTs as well, then checking if recipes are supported isn't reliable since user can opt out of recipes.
There was a problem hiding this comment.
Note: portable types are currently registered with the UDT schema along with all built-in types and includes supportsRecipies capability.
There was a problem hiding this comment.
@lakshmimsft aren't we still relying on typespec to generate schema for portable resources: https://github.com/radius-project/radius/blob/main/typespec/Applications.Datastores/mongoDatabases.tsp? UDT based spec for portable resources is minimal, which I'm assuming was done purely for routing?
| // Run checks if the resource exists, renders the resource, deploys the resource, applies the | ||
| // deployment output to the resource, deletes any resources that are no longer needed, and saves the resource. | ||
| func (c *CreateOrUpdateResource) Run(ctx context.Context, request *ctrl.Request) (ctrl.Result, error) { | ||
| // This code is general and we might be processing an async job for a resource or a scope, so using the general Parse function. |
There was a problem hiding this comment.
I think there is room for abstraction/refactoring both in this controller and deploymentprocessor. This will help improve readability of the code and also deploymentprocessor is using isPortableResource in a weird way where it already has a hardcoded list of portable resource types in the switch case block https://github.com/radius-project/radius/blob/main/pkg/corerp/backend/deployment/deploymentprocessor.go#L517-L565
There was a problem hiding this comment.
Could you please update the PR description to include manual testing that's mentioned in the link issue to validate this change?
- Manually test registration of Recipe for a nonexistent resource type
- Manually test Recipe webhook for a nonexistent resource type. Operation/processing might fail but creation should happen
| @@ -60,16 +65,66 @@ func getDataModel(id resources.ID) (v1.DataModelInterface, error) { | |||
| } | |||
| } | |||
|
|
|||
| func isPortableResource(resourceTypeResourceObj *database.Object) (bool, error) { | |||
There was a problem hiding this comment.
We see this function returns resources that supportRecipes. Should we rename the function to reflect that??
Also where is this being used. 1. We use it in recipe_webhook.go to confirm if the resource runs with recipes 2. we refer to it to see if the resource can have AppId = "". (which needs it's own discussion perhaps)
I don't know is the name isPortableResource is conveying the right meaning.
There was a problem hiding this comment.
I thought portable resources are portable because of recipes and hence the function name. The name existed earlier too, but was a hardcoded listof built in types that supported recipes. I agree its a good idea to discuss. The idea of AppID= "" is because it is an environment resource (again prod env can have its recipe of mongo, test env has its recipe of mongo ). The logic and naming were already existing. The aspect I changed was to remove hardcoding of "portable" resource list and detect it based on manifest.
Description
Since we now have APIs that lets us query the valid resourcetypes and the resource types can include UDTs, we refactored the code to remove the validation of resource types against the static list of redaius resource types.
Type of change
Fixes: https://dev.azure.com/azure-octo/Incubations/_workitems/edit/13653/
Contributor checklist
Please verify that the PR meets the following requirements, where applicable: