Skip to content
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

feat(cli): refactor Delete to new resource manager client #2836

Merged
merged 7 commits into from
Jun 29, 2023
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
32 changes: 18 additions & 14 deletions cli/cmd/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -55,61 +55,62 @@ var resources = resourcemanager.NewRegistry().
resourcemanager.NewClient(
httpClient,
"config", "configs",
resourcemanager.TableConfig{
resourcemanager.WithTableConfig(resourcemanager.TableConfig{
Cells: []resourcemanager.TableCellConfig{
{Header: "ID", Path: "spec.id"},
{Header: "NAME", Path: "spec.name"},
{Header: "ANALYTICS ENABLED", Path: "spec.analyticsEnabled"},
},
},
}),
),
).
Register(
resourcemanager.NewClient(
httpClient,
"analyzer", "analyzers",
resourcemanager.TableConfig{
resourcemanager.WithTableConfig(resourcemanager.TableConfig{
Cells: []resourcemanager.TableCellConfig{
{Header: "ID", Path: "spec.id"},
{Header: "NAME", Path: "spec.name"},
{Header: "ENABLED", Path: "spec.enabled"},
{Header: "MINIMUM SCORE", Path: "spec.minimumScore"},
},
},
}),
),
).
Register(
resourcemanager.NewClient(
httpClient,
"pollingprofile", "pollingprofiles",
resourcemanager.TableConfig{
resourcemanager.WithTableConfig(resourcemanager.TableConfig{
Cells: []resourcemanager.TableCellConfig{
{Header: "ID", Path: "spec.id"},
{Header: "NAME", Path: "spec.name"},
{Header: "STRATEGY", Path: "spec.strategy"},
},
},
}),
),
).
Register(
resourcemanager.NewClient(
httpClient,
"demo", "demos",
resourcemanager.TableConfig{
resourcemanager.WithTableConfig(resourcemanager.TableConfig{
Cells: []resourcemanager.TableCellConfig{
{Header: "ID", Path: "spec.id"},
{Header: "NAME", Path: "spec.name"},
{Header: "TYPE", Path: "spec.type"},
{Header: "ENABLED", Path: "spec.enabled"},
},
},
}),
resourcemanager.WithDeleteEnabled("Demo successfully deleted"),
),
).
Register(
resourcemanager.NewClient(
httpClient,
"datastore", "datastores",
resourcemanager.TableConfig{
resourcemanager.WithTableConfig(resourcemanager.TableConfig{
Cells: []resourcemanager.TableCellConfig{
{Header: "ID", Path: "spec.id"},
{Header: "NAME", Path: "spec.name"},
Expand All @@ -124,27 +125,29 @@ var resources = resourcemanager.NewRegistry().
}
return nil
},
},
}),
resourcemanager.WithDeleteEnabled("DataStore removed. Defaulting back to no-tracing mode"),
),
).
Register(
resourcemanager.NewClient(
httpClient,
"environment", "environments",
resourcemanager.TableConfig{
resourcemanager.WithTableConfig(resourcemanager.TableConfig{
Cells: []resourcemanager.TableCellConfig{
{Header: "ID", Path: "spec.id"},
{Header: "NAME", Path: "spec.name"},
{Header: "DESCRIPTION", Path: "spec.description"},
},
},
}),
resourcemanager.WithDeleteEnabled("Environment successfully deleted"),
),
).
Register(
resourcemanager.NewClient(
httpClient,
"transaction", "transactions",
resourcemanager.TableConfig{
resourcemanager.WithTableConfig(resourcemanager.TableConfig{
Cells: []resourcemanager.TableCellConfig{
{Header: "ID", Path: "spec.id"},
{Header: "NAME", Path: "spec.name"},
Expand Down Expand Up @@ -174,7 +177,8 @@ var resources = resourcemanager.NewRegistry().
}
return nil
},
},
}),
resourcemanager.WithDeleteEnabled("Transaction successfully deleted"),
),
)

Expand Down
62 changes: 35 additions & 27 deletions cli/cmd/delete_cmd.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,40 +3,48 @@ package cmd
import (
"context"
"fmt"
"strings"

"github.com/kubeshop/tracetest/cli/parameters"
"github.com/kubeshop/tracetest/cli/pkg/resourcemanager"
"github.com/spf13/cobra"
)

var deleteParams = &parameters.ResourceIdParams{}

var deleteCmd = &cobra.Command{
GroupID: cmdGroupResources.ID,
Use: fmt.Sprintf("delete %s", strings.Join(parameters.ValidResources, "|")),
Short: "Delete resources",
Long: "Delete resources from your Tracetest server",
PreRun: setupCommand(),
Run: WithResourceMiddleware(func(_ *cobra.Command, args []string) (string, error) {
resourceType := resourceParams.ResourceName
ctx := context.Background()

resourceActions, err := resourceRegistry.Get(resourceType)
if err != nil {
return "", err
}

message, err := resourceActions.Delete(ctx, deleteParams.ResourceID)
if err != nil {
return "", err
}

return fmt.Sprintf("✔ %s", message), nil
}, deleteParams),
PostRun: teardownCommand,
}
var (
deleteParams = &parameters.ResourceIdParams{}
deleteCmd *cobra.Command
)

func init() {
deleteCmd = &cobra.Command{
GroupID: cmdGroupResources.ID,
Use: "delete " + resourceList(),
Short: "Delete resources",
Long: "Delete resources from your Tracetest server",
PreRun: setupCommand(),
Run: WithResourceMiddleware(func(_ *cobra.Command, args []string) (string, error) {
resourceType := resourceParams.ResourceName
ctx := context.Background()

resourceClient, err := resources.Get(resourceType)
if err != nil {
return "", err
}

resultFormat, err := resourcemanager.Formats.Get(output, "yaml")
if err != nil {
return "", err
}

result, err := resourceClient.Delete(ctx, deleteParams.ResourceID, resultFormat)
if err != nil {
return "", err
}

return fmt.Sprintf("✔ %s", result), nil
}, deleteParams),
PostRun: teardownCommand,
}

deleteCmd.Flags().StringVar(&deleteParams.ResourceID, "id", "", "id of the resource to delete")
rootCmd.AddCommand(deleteCmd)
}
38 changes: 35 additions & 3 deletions cli/pkg/resourcemanager/client.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package resourcemanager

import (
"errors"
"fmt"
"io"
"net/http"
Expand All @@ -14,6 +15,7 @@ type client struct {
client *HTTPClient
resourceName string
resourceNamePlural string
deleteSuccessMsg string
tableConfig TableConfig
}

Expand Down Expand Up @@ -45,16 +47,41 @@ func (c HTTPClient) do(req *http.Request) (*http.Response, error) {
return c.client.Do(req)
}

type options func(c *client)

func WithDeleteEnabled(deleteSuccessMssg string) options {
return func(c *client) {
c.deleteSuccessMsg = deleteSuccessMssg
}
}

func WithTableConfig(tableConfig TableConfig) options {
return func(c *client) {
c.tableConfig = tableConfig
}
}

var ErrNotSupportedResourceAction = errors.New("the specified resource type doesn't support the action")

// NewClient creates a new client for a resource managed by the resourceamanger.
// The tableConfig parameter configures how the table view should be rendered.
// This configuration work both for a single resource from a Get, or a ResourceList from a List
func NewClient(httpClient *HTTPClient, resourceName, resourceNamePlural string, tableConfig TableConfig) client {
return client{
func NewClient(
httpClient *HTTPClient,
resourceName, resourceNamePlural string,
opts ...options) client {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

implemented functional options here so it's easier to configure

c := client{
client: httpClient,
resourceName: resourceName,
resourceNamePlural: resourceNamePlural,
tableConfig: tableConfig,
}

for _, opt := range opts {
opt(&c)
}

return c

}

type requestError struct {
Expand All @@ -66,6 +93,11 @@ func (e requestError) Error() string {
return e.Message
}

func isSuccessResponse(resp *http.Response) bool {
// successfull http status codes are 2xx
return resp.StatusCode >= 200 && resp.StatusCode < 300
}

func parseRequestError(resp *http.Response, format Format) error {
body, err := io.ReadAll(resp.Body)
if err != nil {
Expand Down
49 changes: 49 additions & 0 deletions cli/pkg/resourcemanager/delete.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
package resourcemanager

import (
"context"
"fmt"
"net/http"
"net/http/httputil"
)

const VerbDelete Verb = "delete"

func (c client) Delete(ctx context.Context, id string, format Format) (string, error) {
fmt.Println(c.deleteSuccessMsg)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

did you forget to remove this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeap, I did xD

if c.deleteSuccessMsg == "" {
return "", ErrNotSupportedResourceAction
}

url := c.client.url(c.resourceNamePlural, id)
req, err := http.NewRequestWithContext(ctx, http.MethodDelete, url.String(), nil)
if err != nil {
return "", fmt.Errorf("cannot build Delete request: %w", err)
}

d, _ := httputil.DumpRequestOut(req, true)
fmt.Println(string(d))

err = format.BuildRequest(req, VerbDelete)
if err != nil {
return "", fmt.Errorf("cannot build Delete request: %w", err)
}

resp, err := c.client.do(req)
if err != nil {
return "", fmt.Errorf("cannot execute Delete request: %w", err)
}
defer resp.Body.Close()

if !isSuccessResponse(resp) {
err := parseRequestError(resp, format)
reqErr, ok := err.(requestError)
if ok && reqErr.Code == http.StatusNotFound {
return "", fmt.Errorf("Resource %s with ID %s not found", c.resourceName, id)
}

return "", fmt.Errorf("could not Delete resource: %w", err)
}

return c.deleteSuccessMsg, nil
}
2 changes: 1 addition & 1 deletion cli/pkg/resourcemanager/get.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ func (c client) Get(ctx context.Context, id string, format Format) (string, erro
}
defer resp.Body.Close()

if resp.StatusCode != http.StatusOK {
if !isSuccessResponse(resp) {
err := parseRequestError(resp, format)
reqErr, ok := err.(requestError)
if ok && reqErr.Code == http.StatusNotFound {
Expand Down
2 changes: 1 addition & 1 deletion cli/pkg/resourcemanager/list.go
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ func (c client) List(ctx context.Context, opt ListOption, format Format) (string
}
defer resp.Body.Close()

if resp.StatusCode != http.StatusOK {
if !isSuccessResponse(resp) {
err := parseRequestError(resp, format)

return "", fmt.Errorf("could not list resource: %w", err)
Expand Down
2 changes: 1 addition & 1 deletion testing/cli-e2etest/testscenarios/demo/delete_demo_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ func TestDeleteDemo(t *testing.T) {
// Then it should return an error and say that this resource does not exist
result := tracetestcli.Exec(t, "delete demo --id some-demo", tracetestcli.WithCLIConfig(cliConfig))
helpers.RequireExitCodeEqual(t, result, 1)
require.Contains(result.StdErr, "Resource demos with ID some-demo not found") // TODO: update this message to singular
require.Contains(result.StdErr, "Resource demo with ID some-demo not found") // TODO: update this message to singular
schoren marked this conversation as resolved.
Show resolved Hide resolved

// When I try to set up a new demo
// Then it should be applied with success
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ func TestDeleteEnvironment(t *testing.T) {
// Then it should return an error and say that this resource does not exist
result := tracetestcli.Exec(t, "delete environment --id .env", tracetestcli.WithCLIConfig(cliConfig))
helpers.RequireExitCodeEqual(t, result, 1)
require.Contains(result.StdErr, "Resource environments with ID .env not found") // TODO: update this message to singular
require.Contains(result.StdErr, "Resource environment with ID .env not found") // TODO: update this message to singular
schoren marked this conversation as resolved.
Show resolved Hide resolved

// When I try to set up a new environment
// Then it should be applied with success
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ func TestDeleteTransaction(t *testing.T) {
// Then it should return an error and say that this resource does not exist
result := tracetestcli.Exec(t, "delete transaction --id dont-exist", tracetestcli.WithCLIConfig(cliConfig))
helpers.RequireExitCodeEqual(t, result, 1)
require.Contains(result.StdErr, "Resource transactions with ID dont-exist not found") // TODO: update this message to singular
require.Contains(result.StdErr, "Resource transaction with ID dont-exist not found") // TODO: update this message to singular
schoren marked this conversation as resolved.
Show resolved Hide resolved

// When I try to set up a new transaction
// Then it should be applied with success
Expand All @@ -46,5 +46,5 @@ func TestDeleteTransaction(t *testing.T) {
// Then it should return a message saying that the transaction was not found
result = tracetestcli.Exec(t, "delete transaction --id Qti5R3_VR", tracetestcli.WithCLIConfig(cliConfig))
helpers.RequireExitCodeEqual(t, result, 1)
require.Contains(result.StdErr, "Resource transactions with ID Qti5R3_VR not found")
require.Contains(result.StdErr, "Resource transaction with ID Qti5R3_VR not found")
}