-
Notifications
You must be signed in to change notification settings - Fork 97
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
Add functional test for resourceprovider registration (UDT) #8079
Add functional test for resourceprovider registration (UDT) #8079
Conversation
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
0aa8569
to
ed905fd
Compare
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
ed905fd
to
37505b3
Compare
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
37505b3
to
1917123
Compare
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
1917123
to
e613a96
Compare
e613a96
to
74e84e0
Compare
74e84e0
to
9f6f9d7
Compare
Signed-off-by: lakshmimsft <ljavadekar@microsoft.com>
9f6f9d7
to
6f5c5f9
Compare
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
@@ -93,7 +93,7 @@ model ResourceTypeProperties { | |||
provisioningState?: ProvisioningState; | |||
|
|||
@doc("The default api version for the resource type.") | |||
defaultApiVersion: ApiVersionNameString; | |||
defaultApiVersion?: ApiVersionNameString; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might have missed this in the design doc, but would making this optional require code changes? Why? Because now resource type doesn't have a default API version, and the code needs to go and get it from somewhere else or throw an error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For anyone else reviewing, this was a mistake in the initial .tsp. It was always supposed to be optional.
@ytimocin - you're making the assumption that anything reads this field. Nothing does yet.
output, err := cli.RunCommand(ctx, []string{"resource-provider", "list"}) | ||
require.NoError(t, err) | ||
require.Contains(t, output, resourceProviderName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this check for the size of the list?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be brittle due to crosstalk between tests.
require.Contains(t, output, resourceProviderName) | ||
|
||
// Show details of the resource provider. | ||
output, err = cli.RunCommand(ctx, []string{"resource-provider", "show", resourceProviderName, "--output", "json"}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have a default resource provider as in if I run rad rp show
would it show the default one? Just curious...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Description
-Update to Typespec for ResourceType, ResourceProviderSummary to update defaultApiVersion to be optional field.
-Added test for resourceprovider registration which includes calls to create, list and show resource provider.
Type of change
Fixes: Part of #6688
Contributor checklist
Please verify that the PR meets the following requirements, where applicable: