Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion rhoc/pkg/cmd/deployments/deployments.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,9 @@ package deployments
import (
"github.com/bf2fc6cc711aee1a0c2a/cos-tools/rhoc/pkg/cmd/deployments/get"
"github.com/bf2fc6cc711aee1a0c2a/cos-tools/rhoc/pkg/cmd/deployments/list"
"github.com/bf2fc6cc711aee1a0c2a/cos-tools/rhoc/pkg/cmd/deployments/snapshot"
"github.com/bf2fc6cc711aee1a0c2a/cos-tools/rhoc/pkg/cmd/deployments/updateChannel"
"github.com/bf2fc6cc711aee1a0c2a/cos-tools/rhoc/pkg/cmd/deployments/validateSnapshot"
"github.com/bf2fc6cc711aee1a0c2a/cos-tools/rhoc/pkg/util/cmdutil"
"github.com/redhat-developer/app-services-cli/pkg/shared/factory"
"github.com/spf13/cobra"
Expand All @@ -20,7 +22,9 @@ func NewDeploymentsCommand(f *factory.Factory) *cobra.Command {
cmd,
list.NewListCommand(f),
get.NewGetCommand(f),
updateChannel.NewUpdateChannelCommand(f))
updateChannel.NewUpdateChannelCommand(f),
snapshot.NewSnapshotCommand(f),
validateSnapshot.NewValidateSnapshotCommand(f))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

no strong opinion but that be split in 2 snapshot subcommands:

rhoc deployment snapshot create
rhoc deployment snapshot validate

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

makes sense


return cmd
}
10 changes: 10 additions & 0 deletions rhoc/pkg/cmd/deployments/snapshot/deploymentSnapshot.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
package snapshot

import "github.com/bf2fc6cc711aee1a0c2a/cos-tools/rhoc/pkg/api/admin"

type DeploymentSnapshot struct {
ClusterId string
Id string
Kind string
Status admin.ConnectorState
}
176 changes: 176 additions & 0 deletions rhoc/pkg/cmd/deployments/snapshot/snapshot.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,176 @@
package snapshot

import (
"errors"
"fmt"
"github.com/bf2fc6cc711aee1a0c2a/cos-tools/rhoc/pkg/service"
"github.com/bf2fc6cc711aee1a0c2a/cos-tools/rhoc/pkg/util/cmdutil"
"github.com/bf2fc6cc711aee1a0c2a/cos-tools/rhoc/pkg/util/dumper"
"github.com/bf2fc6cc711aee1a0c2a/cos-tools/rhoc/pkg/util/request"
"github.com/redhat-developer/app-services-cli/pkg/shared/factory"
"github.com/spf13/cobra"
"time"
)

const (
CommandName = "snapshot"
CommandAlias = "ss"
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why declaring a constant for this package that is only used once, we could as well directly do:

	cmd := &cobra.Command{
		Short:   "Takes a snapshot of deployments in oldest and ready clusters.",
		Use:      "snapshot",

In case there is a valid reason, why make it public when it is never accessed from outside the package.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I could change it, but that pattern is followed by basically all of the commands.
I don't know what is the reason behind that pattern being used. @lburgazzoli?

If there is no reason behind it, I can change it if we don't mind it being the different one.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

we took this commands originally from rhoas and that was the pattern as I recall it was used also outside the package.

we can do whatever make sense for us.

@lgarciaaco lgarciaaco Nov 22, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The only point I can see having it this way is that you get a good idea about what the package is doing by looking at the top of the file, but even for that purpose there are better ways.

I am just challenging some ideas I know we have been dragging from kas, no strong position whatsoever.


type options struct {
clusterAmount int
outputFile string

f *factory.Factory
}

func NewSnapshotCommand(f *factory.Factory) *cobra.Command {
opts := options{
f: f,
}

cmd := &cobra.Command{
Short: "Takes a snapshot of deployments in oldest and ready clusters.",
Use: CommandName,
Aliases: []string{CommandAlias},
Args: cobra.NoArgs,
PreRunE: func(cmd *cobra.Command, args []string) error {
if opts.clusterAmount < 1 || opts.clusterAmount > 30 {
return errors.New("clusterAmount must be between 1 and 30")
}
return nil
},
RunE: func(cmd *cobra.Command, args []string) error {
return run(&opts)
},
}

cmdutil.AddClusterAmount(cmd, &opts.clusterAmount)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

maybe there should be a way to define for which cluster you want to get the snapshot ? my understading is that you can define a number of cluster for which you want a snapshot but that would be unreliable

@rinaldodev rinaldodev Nov 16, 2022

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

the reasoning was querying for X clusters with status=ready and sorted by age.
Right now I'd rather change the validation mechanism to parse the CSV and query for the same clusters afterwards, as to prevent that from happening.

adding the option to set specific clusters is a valid option and can also be done, but right now I just wanted something more straightforward than figuring out know which are the "important clusters" beforehand.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

that's would be fine, main reason was about the consistency between the save and validate

cmdutil.AddOutputFile(cmd, &opts.outputFile)

return cmd
}

func run(opts *options) error {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why a pointer here? I think implementing this function by value achieve the same goal

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Same as above

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

same as above :)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Haha ... I've noticed from kas they tend to use pointers everywhere. Imho pointers are a tool in a wide toolset and should not be used just because it is available. The default approach should be to use value implementation instead of reference.

I know it is a copy and paste issue and I am bringing this up to discussion because I saw it earlier this week, sorry for using your PR as a platform :-)

fmt.Fprintln(opts.f.IOStreams.Out, "Starting snapshot creation")

if opts.outputFile == "" {
defaultFilename := fmt.Sprintf("snapshot%s.csv", time.Now().Format("20060102150405"))
fmt.Fprintf(opts.f.IOStreams.Out, "outputFile wasn't set, results will be written to %s\n", defaultFilename)
opts.outputFile = defaultFilename
}

c, err := service.NewAdminClient(&service.Config{
F: opts.f,
})
if err != nil {
return err
}

fmt.Fprintln(opts.f.IOStreams.Out, "Querying for clusters..")

clusterListOpts := request.ListOptions{
Page: 0,
Limit: opts.clusterAmount,
AllPages: false,
OrderBy: "created_at",
Search: "state=ready",
}
clusters, err := service.ListClusters(c, clusterListOpts)
if err != nil {
return err
}

fmt.Fprintf(opts.f.IOStreams.Out, "Got %d clusters\n", clusters.Size)

var snapshots []DeploymentSnapshot
for _, cluster := range clusters.Items {
fmt.Fprintf(opts.f.IOStreams.Out, "Querying for deployments in cluster %s..\n", cluster.Id)

deployListOpts := request.ListDeploymentsOptions{
ListOptions: request.ListOptions{
AllPages: false,
Limit: 30,
Page: 0,
},
ChannelUpdate: false,
DanglingDeployments: false,
}

deployments, err := service.ListDeploymentsForCluster(c, deployListOpts, cluster.Id)
if err != nil {
return err
}

fmt.Fprintf(opts.f.IOStreams.Out, "Got %d deployments in cluster %s\n", deployments.Size, cluster.Id)

for _, deploy := range deployments.Items {
snapshots = append(snapshots, DeploymentSnapshot{
ClusterId: cluster.Id,
Id: deploy.Id,
Kind: deploy.Spec.ConnectorTypeId,
Status: deploy.Status.Phase,
})
}
}

outputWriter, err := cmdutil.NewOutputFileWriter(opts.outputFile)
if err != nil {
return err
}
defer outputWriter.Close()

fmt.Fprintf(opts.f.IOStreams.Out, "Creating snapshot with %d connector deployments\n", len(snapshots))
err = dumper.DumpTable(
dumper.TableConfig[DeploymentSnapshot]{
Style: dumper.TableStyleCSV,
Wide: false,
Columns: []dumper.Column[DeploymentSnapshot]{
{
Name: "ClusterID",
Wide: false,
Getter: func(in *DeploymentSnapshot) dumper.Row {
return dumper.Row{
Value: in.ClusterId,
}
},
},
{
Name: "ID",
Wide: false,
Getter: func(in *DeploymentSnapshot) dumper.Row {
return dumper.Row{
Value: in.Id,
}
},
},
{
Name: "ConnectorTypeID",
Wide: false,
Getter: func(in *DeploymentSnapshot) dumper.Row {
return dumper.Row{
Value: in.Kind,
}
},
},
{
Name: "Status",
Wide: false,
Getter: func(in *DeploymentSnapshot) dumper.Row {
return dumper.Row{
Value: string(in.Status),
}
},
},
},
},
outputWriter,
snapshots,
)
if err != nil {
return err
}
fmt.Fprintf(opts.f.IOStreams.Out, "Snapshot created with %d connector deployments\n", len(snapshots))

return nil
}
Loading