-
Notifications
You must be signed in to change notification settings - Fork 100
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
[UDT] add create resource type command #8104
Conversation
b81faec
to
4c00940
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8104 +/- ##
==========================================
- Coverage 59.94% 59.75% -0.19%
==========================================
Files 589 590 +1
Lines 39237 39487 +250
==========================================
+ Hits 23519 23597 +78
- Misses 13981 14146 +165
- Partials 1737 1744 +7 ☔ View full report in Codecov by Sentry. |
09246ab
to
8b5d45f
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... |
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... |
492c575
to
76430a0
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... |
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... |
|
||
// If resource provider does not exist or if there is any other error, first register the resource provider. | ||
response, err := r.UCPClientFactory.NewResourceProvidersClient().Get(ctx, "local", r.ResourceProvider.Name, nil) | ||
if err != nil { |
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.
The error can also be something other than Resource Provider not Found
, right?
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.
yes. Currently, if there are errors, approach is to recreate. @rynowak do you see any problems with this approach?
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.
I think we should only create if the error is 404 and fail if it is something else.
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.
Added changes to return error if it is not 404.
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.
I think we should only create if the error is 404 and fail if it is something else.
Added changes to return error if it is not 404.
|
||
// If resource provider does not exist or if there is any other error, first register the resource provider. | ||
response, err := r.UCPClientFactory.NewResourceProvidersClient().Get(ctx, "local", r.ResourceProvider.Name, nil) | ||
if err != nil { |
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.
I think we should only create if the error is 404 and fail if it is something else.
… case to capture the flow.
55a1c0d
to
1469009
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... |
return err | ||
} | ||
|
||
// get the existing location resource and update it with new resource type. We have to revisit this code once schema is finalized and validated. |
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.
nit: remove
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.
I think this comment is useful because it indicates revisiting the code once schema validation comes in.
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... |
Description
add rad resource-type create
Type of change
Partially Fixes: #issue_number
Contributor checklist
Please verify that the PR meets the following requirements, where applicable: