Add delete and list command #101

greghaynes · 2023-12-05T23:46:08Z

This isnt perfect, but this should handle the 90% case for when users dont want to have to install kind separately.

nabuskey · 2023-12-06T17:11:20Z

pkg/cmd/delete/root.go

+	if err != nil {
+		return err
+	}
+	if err := cluster.Delete(); err != nil {


Do we get an error if we call delete on a cluster that doesn't exist? If it does return an error, do we want to check it exists then exit with status 0?

cmoulliard · 2023-12-13T15:19:50Z

pkg/cmd/delete/root.go

+}
+
+func init() {
+	DeleteCmd.PersistentFlags().StringVar(&buildName, "buildName", "localdev", "Name for build (Prefix for kind cluster name, pod names, etc).")


The text "Name for build (Prefix for kind cluster name, pod names, etc)." is confusing.
I suggest "Name of the kind cluster to be deleted."

nabuskey · 2024-02-15T23:53:30Z

pkg/cmd/list/root.go

+}
+
+func init() {
+	ListCmd.PersistentFlags().StringVar(&buildName, "buildName", "localdev", "Name of the kind cluster to be deleted.")


Should be build-name instead of buildName because we use kebab case everywhere else.

nimakaviani · 2024-02-19T16:16:24Z

pkg/cmd/list/root.go

+	Use:   "list",
+	Short: "List idp clusters",
+	Long:  ``,
+	RunE:  delete,


function name needs to be changed

nimakaviani · 2024-02-19T16:16:32Z

pkg/cmd/list/root.go

+	ctrl.SetLogger(zap.New(zap.UseFlagOptions(&opts)))
+}
+
+func delete(cmd *cobra.Command, args []string) error {


same as above

nimakaviani · 2024-02-19T16:19:06Z

pkg/cmd/list/root.go

+}
+
+func init() {
+	ListCmd.PersistentFlags().StringVar(&buildName, "build-name", "localdev", "Name of the kind cluster to be deleted.")


why require a build-name if listingg clusters? also the description needs fixing

nimakaviani · 2024-02-19T16:19:31Z

pkg/cmd/list/root.go

+		return errors.Wrapf(err, "failed to delete cluster %q", buildName)
+	}
+
+	fmt.Printf("Clusters: %v\n", clusters)


maybe a pretty print?

Whats that?

greghaynes · 2024-04-25T00:59:10Z

@nimakaviani @nabuskey Sorry I dropped this - it should be ready for re-review

nabuskey · 2024-04-25T16:22:28Z

pkg/cmd/delete/root.go

+	zapfs := flag.NewFlagSet("zap", flag.ExitOnError)
+	opts := zap.Options{
+		Development: true,
+	}
+	opts.BindFlags(zapfs)
+	DeleteCmd.Flags().AddGoFlagSet(zapfs)
+


Let's use

idpbuilder/pkg/cmd/helpers/logger.go

Line 19 in 8094f56

func SetLogger() error {

instead?

nabuskey · 2024-04-25T16:25:43Z

pkg/cmd/delete/root.go

+}
+
+func delete(cmd *cobra.Command, args []string) error {
+	provider := cluster.NewProvider(cluster.ProviderWithDocker())


Wonder if we can use DetectNodeProvider() here? Not sure 100% but might be possible. Just in case someone created one with another provider. #182

cmoulliard · 2024-05-02T09:17:11Z

Can you resolve the conflicts (rebase) please ?

cmoulliard

Please rebase the code due to conflicts
Use a printOutPut function to list the clusters. Ideally the cluster listed should mention in separate columns: core packages and custom packages if available

// Example from list secrets
func printOutput(templatePath string, outWriter io.Writer, data []any, format string) error {
	switch format {
	case "json":
		b, err := json.MarshalIndent(data, "", "  ")
		if err != nil {
			return err
		}
		b = append(b, []byte("\n")...)
		_, err = outWriter.Write(b)
		return err
	case "yaml":
		b, err := yaml.Marshal(data)
		if err != nil {
			return err
		}
		_, err = outWriter.Write(b)
		return err
	case "":
		return renderTemplate(templatePath, outWriter, data)
	default:
		return fmt.Errorf("output format %s is not supported", format)
	}
}

greghaynes · 2024-05-21T18:24:26Z

@cmoulliard I moved this to match Kind's upstream behavior - for a 'get clusters' command I'm not sure its necessary to have templates/json/etc. IME its pretty common to simply list the clusters (this is very useful for e.g. shell tooling integration). If we want to add extra formatting dont let me stop anyone from making that an optional argument.

greghaynes · 2024-05-21T18:25:12Z

@nabuskey @nimakaviani Can you all re-review this please? thanks

nimakaviani

minor comment on the naming of the command, otherwise looks good.

nimakaviani · 2024-05-23T17:53:13Z

pkg/cmd/get/clusters/clusters.go

+)
+
+var ClustersCmd = &cobra.Command{
+	Use:   "clusters",


not to be too picky here but looks like the other commands are verbs and this one is a noun. shall we keep it as list like it used to be? or get maybe?

nimakaviani · 2024-06-02T07:50:13Z

@greghaynes once you resolve the conflict, this should be good to go.

Add list command too Remove changes to delete routines Update buildName to build-name Fix rebase miss Fix list to not use build name arg Fix delete cmd function name Bump kubebuilder version Signed-off-by: Greg Haynes <[email protected]> Signed-off-by: Manabu McCloskey <[email protected]>

Signed-off-by: Greg Haynes <[email protected]> Signed-off-by: Manabu McCloskey <[email protected]>

nimakaviani

LGTM

greghaynes requested a review from nabuskey December 5, 2023 23:46

greghaynes force-pushed the delete-command branch from 3c96832 to 4506ca9 Compare December 5, 2023 23:46

nabuskey reviewed Dec 6, 2023

View reviewed changes

cmoulliard reviewed Dec 13, 2023

View reviewed changes

greghaynes changed the title ~~Add delete command~~ Add delete and list command Feb 15, 2024

greghaynes force-pushed the delete-command branch 2 times, most recently from 809287b to ee9a96f Compare February 15, 2024 22:59

nabuskey requested changes Feb 15, 2024

View reviewed changes

nimakaviani requested changes Feb 19, 2024

View reviewed changes

greghaynes force-pushed the delete-command branch 2 times, most recently from 1c76742 to 9a73bd3 Compare April 25, 2024 00:58

nabuskey reviewed Apr 25, 2024

View reviewed changes

nimakaviani requested a review from cmoulliard April 30, 2024 00:07

cmoulliard reviewed May 2, 2024

View reviewed changes

greghaynes force-pushed the delete-command branch from 32d7949 to d19749c Compare May 21, 2024 16:57

greghaynes force-pushed the delete-command branch from d31dbcf to 0c50a02 Compare May 21, 2024 18:24

nimakaviani reviewed May 23, 2024

View reviewed changes

nabuskey requested a review from a team as a code owner July 30, 2024 23:07

nabuskey force-pushed the delete-command branch 2 times, most recently from aeace38 to 868dd4f Compare July 30, 2024 23:56

greghaynes and others added 3 commits July 30, 2024 23:57

Use SetLogger and auto detet node provider for kind

450ede5

Signed-off-by: Greg Haynes <[email protected]> Signed-off-by: Manabu McCloskey <[email protected]>

Move list to get clusters command, matching Kind

1b8b238

Signed-off-by: Greg Haynes <[email protected]> Signed-off-by: Manabu McCloskey <[email protected]>

nabuskey force-pushed the delete-command branch from 868dd4f to 1b8b238 Compare July 30, 2024 23:57

nimakaviani approved these changes Jul 31, 2024

View reviewed changes

nabuskey approved these changes Jul 31, 2024

View reviewed changes

nabuskey merged commit af755fd into main Jul 31, 2024
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add delete and list command #101

Add delete and list command #101

greghaynes commented Dec 5, 2023 •

edited

Loading

nabuskey Dec 6, 2023

cmoulliard Dec 13, 2023

nabuskey Feb 15, 2024

nimakaviani Feb 19, 2024

nimakaviani Feb 19, 2024

nimakaviani Feb 19, 2024

nimakaviani Feb 19, 2024

greghaynes Apr 25, 2024

greghaynes commented Apr 25, 2024

nabuskey Apr 25, 2024

nabuskey Apr 25, 2024

cmoulliard commented May 2, 2024

cmoulliard left a comment

greghaynes commented May 21, 2024

greghaynes commented May 21, 2024

nimakaviani left a comment

nimakaviani May 23, 2024

nimakaviani commented Jun 2, 2024

nimakaviani left a comment

Add delete and list command #101

Add delete and list command #101

Conversation

greghaynes commented Dec 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greghaynes commented Apr 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmoulliard commented May 2, 2024

cmoulliard left a comment

Choose a reason for hiding this comment

greghaynes commented May 21, 2024

greghaynes commented May 21, 2024

nimakaviani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nimakaviani commented Jun 2, 2024

nimakaviani left a comment

Choose a reason for hiding this comment

greghaynes commented Dec 5, 2023 •

edited

Loading