Update CLI client code and add tests #7594
Conversation
| ListAllResourcesByEnvironment(ctx context.Context, environmentName string) ([]generated.GenericResource, error) | ||
| ShowResource(ctx context.Context, resourceType string, resourceName string) (generated.GenericResource, error) | ||
| DeleteResource(ctx context.Context, resourceType string, resourceName string) (bool, error) | ||
| // ListResourcesOfType lists all resources of a given type in the configured scope. |
There was a problem hiding this comment.
This is where I did a major pass for consistency of the APIs.
| applicationResourceClientFactory func(scope string) (applicationResourceClient, error) | ||
| environmentResourceClientFactory func(scope string) (environmentResourceClient, error) | ||
| resourceGroupClientFactory func() (resourceGroupClient, error) | ||
| capture func(ctx context.Context, capture **http.Response) context.Context |
There was a problem hiding this comment.
Please review this file in detail. Each of these functions have been updated to be more consistent (naming, structure, etc).
Adding mocks was pretty complex.
pkg/cli/clients/management.go
Outdated
| } | ||
|
|
||
| return corerpv20231001.RecipeGetMetadataResponse(resp.RecipeGetMetadataResponse), nil | ||
| return amc.capture(ctx, capture) |
There was a problem hiding this comment.
Mocking the behavior of runtime.WithCaptureResponse is ugllllly....
| ucpv20231001 "github.com/radius-project/radius/pkg/ucp/api/v20231001preview" | ||
| ) | ||
|
|
||
| // We define interfaces so we can mock the interactions with the Radius API. These |
There was a problem hiding this comment.
This is everything that was required to mock the client code 😓
| @@ -50,7 +50,7 @@ func Test_Validate(t *testing.T) { | |||
| }, | |||
| ConfigureMocks: func(mocks radcli.ValidateMocks) { | |||
| mocks.ApplicationManagementClient.EXPECT(). | |||
| ShowApplication(gomock.Any(), "test-app"). | |||
| GetApplication(gomock.Any(), "test-app"). | |||
There was a problem hiding this comment.
The changes to the client API resulted in changes to most commands, but they are just renames.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7594 +/- ##
==========================================
+ Coverage 60.30% 61.03% +0.72%
==========================================
Files 517 517
Lines 26432 26511 +79
==========================================
+ Hits 15941 16180 +239
+ Misses 9141 8911 -230
- Partials 1350 1420 +70 ☔ View full report in Codecov by Sentry. |
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... |
| GetApplication(ctx context.Context, applicationNameOrID string) (corerp.ApplicationResource, error) | ||
|
|
||
| // GetApplicationGraph retrieves the application graph of an application by its name (or id). | ||
| GetApplicationGraph(ctx context.Context, applicationNameOrID string) (corerp.ApplicationGraphResponse, error) |
There was a problem hiding this comment.
nice to see we can use ID or name everywhere.
There was a problem hiding this comment.
hmm, does this make -g flag somewhat redundant? that now would make sense only if the "name" was used, not id right?
There was a problem hiding this comment.
Not really. We'd still need to update the rest of the CLI to pass in a resource ID instead of a name. Right now the CLI only works with names and assumes the "default scope".
So this change only really improves this specific layer.
| ShowResource(ctx context.Context, resourceType string, resourceName string) (generated.GenericResource, error) | ||
| DeleteResource(ctx context.Context, resourceType string, resourceName string) (bool, error) | ||
| // ListResourcesOfType lists all resources of a given type in the configured scope. | ||
| ListResourcesOfType(ctx context.Context, resourceType string) ([]generated.GenericResource, error) |
There was a problem hiding this comment.
Did we decide to remove the ability to have a single call that fetches all resources in an application?
There was a problem hiding this comment.
never mind, I just realized its still there. We are introducing a new ability to list all resources of type ( not filtered by env or app).
rynowak
force-pushed
the
cli-client-groups
branch
from
7d2a307
to
5c98df9
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... |
|
Looks like there's another bug for me to fix that's blocking functional tests. This is the downside of mock-based testing, you could have bugs in the code that's mocked out. |
rynowak
force-pushed
the
cli-client-groups
branch
from
5c98df9
to
90bf38a
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... |
rynowak
force-pushed
the
cli-client-groups
branch
from
90bf38a
to
0c49ad0
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... |
|
FYI - I found #7597 while troubleshooting this code and added a workaround for it. |
rynowak
force-pushed
the
cli-client-groups
branch
from
0c49ad0
to
f657d97
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... |
This change is a major update to the CLI's client code for interacting with the Radius API. The client grew organically over time as we added features. Since it's client code it wasn't well tested because testing it would require a lot of mocking. Unfortunately in some places we *do* have complex logic in this code and unfortunately we also have some bugs. I'm working on another fix to address radius-project#7520, and encountered a lot of limitations with the client code so I decided to fix it. This change addresses the following: - Reviewed the API and made updates for consistency. - Added tests for ALL of the functions on the client. - Updated each API to accept either a resource ID or a resource name (needed for radius-project#7520). This update does the Signed-off-by: Ryan Nowak <nowakra@gmail.com>
rynowak
force-pushed
the
cli-client-groups
branch
from
f657d97
to
5211238
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... |
| ListResourceGroups(ctx context.Context, planeName string) ([]ucp_v20231001preview.ResourceGroupResource, error) | ||
|
|
||
| // GetResourceGroup retrieves a resource group by its name. | ||
| GetResourceGroup(ctx context.Context, planeName string, resourceGroupName string) (ucp_v20231001preview.ResourceGroupResource, error) |
There was a problem hiding this comment.
Are the resourcegroup APIs not with NameOrID for a reason?
There was a problem hiding this comment.
I thought about this decided against. Right now they take the planeName and the resourceGroupName which means that they can always build the resource id fully. So taking in a resourceGroupNameOrId would make the plane name redundant.
There was a problem hiding this comment.
Does this sound right? any better ideas?
There was a problem hiding this comment.
This is fine too but I feel it would be better if all APIs accept similar input formats.
There was a problem hiding this comment.
What do you think the right signature is?
An API like GetEnvironment(ctx, nameOrId) works because the client has a notion of "default scope" that can be prepended to the name.
What do you think the behavior of GetResourceGroup(ctx, nameOrId) should be?
| GetResource(ctx context.Context, resourceType string, resourceNameOrID string) (generated.GenericResource, error) | ||
|
|
||
| // DeleteResource deletes a resource by its type and name (or id). | ||
| DeleteResource(ctx context.Context, resourceType string, resourceNameOrID string) (bool, error) |
There was a problem hiding this comment.
Common comment for all the APIs: There are no tests to test these APIs by ID
There was a problem hiding this comment.
I'll double check this. I don't plan to add a "name test" + "id test" for every API. The difference between the name case and the ID case are handled in common code.
There was a problem hiding this comment.
I looked again and there's a good mix of tests that use names and tests that use IDs. It feels like a lot of wasted effort to test both cases for each of the 15 functions. They all call into the same code to resolve the full id.
Please let me know if there's something specific you want to see added.
|
I'm going to merge this. @vinayada1 - if you have additional feedback for me to address I'll do it in a follow-up 👍 |
Description
This change is a major update to the CLI's client code for interacting with the Radius API.
The client grew organically over time as we added features. Since it's client code it wasn't well tested because testing it would require a lot of mocking. Unfortunately in some places we do have complex logic in this code and unfortunately we also have some bugs.
I'm working on another fix to address #7520, and encountered a lot of limitations with the client code so I decided to fix it.
This change addresses the following:
Type of change