Improve the code to print the secrets using printTable #464

cmoulliard · 2024-12-19T14:04:05Z

Improve the code to print the secrets using the cli-runtime line and printTable.
Refactor the code to use the print functions from the util package
See: [Suggestion] Enhance how we print the secret to show them using as format: Table, Yaml or JSON #463

…printTable. cnoe-io#463 Signed-off-by: cmoulliard <[email protected]>

Signed-off-by: cmoulliard <[email protected]>

cmoulliard · 2024-12-19T15:25:10Z

Question: Should we for the core package secrets display the information as such ?

[
  {
    "name": "argocd-initial-admin-secret",
    "namespace": "argocd",
    "data": {
      "password": "cP0HzmKo8vbzpa5L",
      "username": "admin"
    }
  }
]

OR

[
  {
    "name": "argocd-initial-admin-secret",
    "namespace": "argocd",
    "username": "admin",
    "password": "cP0HzmKo8vbzpa5L",
    }
  }
]

cmoulliard · 2024-12-19T16:51:03Z

We should review in a seperate PR how we will display the k=v data of the secrets labelized with cnoe.io/cli-secret: "true" as currently I do simply

❯ ./idpbuilder get secrets
NAME                          NAMESPACE   USERNAME     PASSWORD                                   TOKEN                                      DATA
argocd-initial-admin-secret   argocd      admin        cP0HzmKo8vbzpa5L                                                                      
gitea-credential              gitea       giteaAdmin   8./6=O(C7{Vy|]I%V!#]]_/7+,j>/WGXgrX?64rf   6ff17d317da8ef57e45bb4f9a91ab99bfe5d9c72   
cnoe                          default                                                                                                        user1=charles, user2=Manabu...

…r: isCore to make the distinction betweek the core secrets and secrets having the CNOE label Signed-off-by: cmoulliard <[email protected]>

cmoulliard · 2024-12-19T17:16:53Z

To be investigated as test is failing.

Fixed

…rmats supported Signed-off-by: cmoulliard <[email protected]>

punkwalker · 2024-12-19T19:19:51Z

Question: Should we for the core package secrets display the information as such ?

[
  {
    "name": "argocd-initial-admin-secret",
    "namespace": "argocd",
    "data": {
      "password": "cP0HzmKo8vbzpa5L",
      "username": "admin"
    }
  }
]

OR

[
  {
    "name": "argocd-initial-admin-secret",
    "namespace": "argocd",
    "username": "admin",
    "password": "cP0HzmKo8vbzpa5L",
    }
  }
]

I vote for 1st option. This way we can have flexibility for complex fields.

punkwalker

nit: I suggest that we create a separate Printer package and Printer Interface.

punkwalker · 2024-12-19T19:00:38Z

pkg/cmd/get/root.go

@@ -22,7 +22,7 @@ func init() {
 	GetCmd.AddCommand(ClustersCmd)
 	GetCmd.AddCommand(SecretsCmd)
 	GetCmd.PersistentFlags().StringSliceVarP(&packages, "packages", "p", []string{}, "names of packages.")
-	GetCmd.PersistentFlags().StringVarP(&outputFormat, "output", "o", "", "Output format. json or yaml.")
+	GetCmd.PersistentFlags().StringVarP(&outputFormat, "output", "o", "", "Output format: table (default if not specified), json or yaml.")


let's name the default as "table" instead of "".

I agree. Fixed: 288284e

punkwalker · 2024-12-19T19:04:20Z

pkg/cmd/get/clusters.go

+func printClustersOutput(outWriter io.Writer, clusters []Cluster, format string) error {
+	switch format {
+	case "json":
+		return util.PrintDataAsJson(clusters, outWriter)
+	case "yaml":
+		return util.PrintDataAsYaml(clusters, outWriter)
+	case "":
+		return util.PrintTable(generateClusterTable(clusters), outWriter)
+	default:
+
+		return fmt.Errorf("output format %s is not supported", format)


Can we combine this function for secret and cluster both and add it in utils package to avoid duplicate code?

Can we combine this function for secret and cluster both and add it in utils package to avoid duplicate code?

Here is what I did which is still a WIP: 580042f, 13b4ad9

Improvements are welcome ;-) @punkwalker

punkwalker · 2024-12-19T19:24:02Z

pkg/cmd/get/secrets.go

+func generateSecret(s v1.Secret, isCoreSecret bool) Secret {
+	secret := Secret{
 		Name:      s.Name,
 		Namespace: s.Namespace,


We should name this populateSecrets instead of generateSecret. I felt we are creating a new secret.

Fixed: 813c0cd

punkwalker · 2024-12-19T19:26:17Z

pkg/cmd/get/secrets.go


 type TemplateData struct {
 	Name      string            `json:"name"`
 	Namespace string            `json:"namespace"`
 	Data      map[string]string `json:"data"`
 }

+type Secret struct {
+	isCore    bool


Is the use of isCore field just to control the printing of core package fields? Can we do that without this?

Is the use of isCore field just to control the printing of core package fields?

It is used to figure out when we have a core package secret containing as values: username, password and token (for gitea)

Can we do that without this?

Not really

cmoulliard · 2024-12-20T08:00:14Z

I vote for 1st option. This way we can have flexibility for complex fields.

This is what I did

Signed-off-by: cmoulliard <[email protected]>

cmoulliard · 2024-12-20T08:54:30Z

I suggest that we create a separate Printer package

Great idea. Fixed: 771fd89

cmoulliard · 2024-12-20T08:55:01Z

and Printer Interface.

Why ? Can you elaborate ?

Signed-off-by: cmoulliard <[email protected]>

… Node, etc. and created printer methods. WIP Signed-off-by: cmoulliard <[email protected]>

Signed-off-by: cmoulliard <[email protected]>

nabuskey · 2024-12-20T17:42:08Z

Question: Should we for the core package secrets display the information as such ?

[
  {
    "name": "argocd-initial-admin-secret",
    "namespace": "argocd",
    "data": {
      "password": "cP0HzmKo8vbzpa5L",
      "username": "admin"
    }
  }
]

OR

[
  {
    "name": "argocd-initial-admin-secret",
    "namespace": "argocd",
    "username": "admin",
    "password": "cP0HzmKo8vbzpa5L",
    }
  }
]

I prefer the second format but like you said, we can discuss that in another PR / issue.

punkwalker · 2024-12-20T22:13:27Z

and Printer Interface.

Why ? Can you elaborate ?

We can address this later. As of now, we can merge this PR

cmoulliard · 2024-12-23T13:36:13Z

Should we wait an approval from @nabuskey @blakeromano ? @punkwalker

This comment was marked as resolved.

Sign in to view

cmoulliard added 2 commits December 19, 2024 15:37

Improve the code to print the secrets using the cli-runtime line and …

9fb9c76

…printTable. cnoe-io#463 Signed-off-by: cmoulliard <[email protected]>

Pass the writer using stdout to the functions

666bfc7

Signed-off-by: cmoulliard <[email protected]>

cmoulliard force-pushed the new-secret-print-table branch from 497a15a to 666bfc7 Compare December 19, 2024 14:38

Fix wrong writer used: io.Discard => buffer

cc43fcc

Signed-off-by: cmoulliard <[email protected]>

Fix the issue with the test case: secrets_test.go. Set a new paramete…

08cbf2d

…r: isCore to make the distinction betweek the core secrets and secrets having the CNOE label Signed-off-by: cmoulliard <[email protected]>

cmoulliard requested review from blakeromano and nabuskey December 19, 2024 17:20

Improve the description of the flag: outputFormat to mention the 3 fo…

913f60a

…rmats supported Signed-off-by: cmoulliard <[email protected]>

punkwalker requested changes Dec 19, 2024

View reviewed changes

cmoulliard added 2 commits December 20, 2024 09:44

Set the default outputFormat as: table

288284e

Signed-off-by: cmoulliard <[email protected]>

Moving the print functions to a new package: printer

771fd89

Signed-off-by: cmoulliard <[email protected]>

cmoulliard added 4 commits December 20, 2024 09:57

Rename generateSecret to populateSecret

813c0cd

Signed-off-by: cmoulliard <[email protected]>

Moving the function PrintOutput to the printer package. WIP

580042f

Signed-off-by: cmoulliard <[email protected]>

Created entity package to manage the internal types: Secret, Cluster,…

13b4ad9

… Node, etc. and created printer methods. WIP Signed-off-by: cmoulliard <[email protected]>

Replace the emtpty format value with the default: table

358c83b

Signed-off-by: cmoulliard <[email protected]>

cmoulliard requested a review from punkwalker December 20, 2024 12:59

cmoulliard added 2 commits December 20, 2024 14:22

Move the code to their corresponding go file

3a2c119

Signed-off-by: cmoulliard <[email protected]>

Remove the need to use the type to access the function

4127914

Signed-off-by: cmoulliard <[email protected]>

punkwalker approved these changes Dec 20, 2024

View reviewed changes

blakeromano approved these changes Dec 23, 2024

View reviewed changes

cmoulliard merged commit 6f834bc into cnoe-io:main Dec 24, 2024
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve the code to print the secrets using printTable #464

Improve the code to print the secrets using printTable #464

cmoulliard commented Dec 19, 2024

This comment was marked as resolved.

cmoulliard commented Dec 19, 2024

cmoulliard commented Dec 19, 2024

cmoulliard commented Dec 19, 2024

punkwalker commented Dec 19, 2024 •

edited

Loading

punkwalker left a comment

punkwalker Dec 19, 2024

cmoulliard Dec 20, 2024

punkwalker Dec 19, 2024

cmoulliard Dec 20, 2024 •

edited

Loading

punkwalker Dec 19, 2024

cmoulliard Dec 20, 2024

punkwalker Dec 19, 2024

cmoulliard Dec 20, 2024

cmoulliard Dec 20, 2024

cmoulliard commented Dec 20, 2024

cmoulliard commented Dec 20, 2024 •

edited

Loading

cmoulliard commented Dec 20, 2024

nabuskey commented Dec 20, 2024

punkwalker commented Dec 20, 2024

cmoulliard commented Dec 23, 2024

Improve the code to print the secrets using printTable #464

Improve the code to print the secrets using printTable #464

Conversation

cmoulliard commented Dec 19, 2024

This comment was marked as resolved.

cmoulliard commented Dec 19, 2024

cmoulliard commented Dec 19, 2024

cmoulliard commented Dec 19, 2024

punkwalker commented Dec 19, 2024 • edited Loading

punkwalker left a 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

cmoulliard Dec 20, 2024 • 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

cmoulliard commented Dec 20, 2024

cmoulliard commented Dec 20, 2024 • edited Loading

cmoulliard commented Dec 20, 2024

nabuskey commented Dec 20, 2024

punkwalker commented Dec 20, 2024

cmoulliard commented Dec 23, 2024

punkwalker commented Dec 19, 2024 •

edited

Loading

cmoulliard Dec 20, 2024 •

edited

Loading

cmoulliard commented Dec 20, 2024 •

edited

Loading