Improve the code to print the secrets using printTable by cmoulliard · Pull Request #464 · cnoe-io/idpbuilder

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 <cmoulliard@redhat.com>

Signed-off-by: cmoulliard <cmoulliard@redhat.com>

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 <cmoulliard@redhat.com>

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

To be investigated as test is failing.

Fixed

…rmats supported Signed-off-by: cmoulliard <cmoulliard@redhat.com>

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

 	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 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 <cmoulliard@redhat.com>

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 <cmoulliard@redhat.com>

… Node, etc. and created printer methods. WIP Signed-off-by: cmoulliard <cmoulliard@redhat.com>

Signed-off-by: cmoulliard <cmoulliard@redhat.com>

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 <cmoulliard@redhat.com>

Pass the writer using stdout to the functions

666bfc7

Signed-off-by: cmoulliard <cmoulliard@redhat.com>

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 <cmoulliard@redhat.com>

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 <cmoulliard@redhat.com>

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 <cmoulliard@redhat.com>

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 <cmoulliard@redhat.com>

Moving the print functions to a new package: printer

771fd89

Signed-off-by: cmoulliard <cmoulliard@redhat.com>

cmoulliard added 4 commits December 20, 2024 09:57

Rename generateSecret to populateSecret

813c0cd

Signed-off-by: cmoulliard <cmoulliard@redhat.com>

Moving the function PrintOutput to the printer package. WIP

580042f

Signed-off-by: cmoulliard <cmoulliard@redhat.com>

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

13b4ad9

… Node, etc. and created printer methods. WIP Signed-off-by: cmoulliard <cmoulliard@redhat.com>

Replace the emtpty format value with the default: table

358c83b

Signed-off-by: cmoulliard <cmoulliard@redhat.com>

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 <cmoulliard@redhat.com>

Remove the need to use the type to access the function

4127914

Signed-off-by: cmoulliard <cmoulliard@redhat.com>

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

Conversation

cmoulliard commented Dec 19, 2024

Uh oh!

This comment was marked as resolved.

cmoulliard commented Dec 19, 2024

Uh oh!

cmoulliard commented Dec 19, 2024

Uh oh!

cmoulliard commented Dec 19, 2024

Uh oh!

punkwalker commented Dec 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

punkwalker left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cmoulliard Dec 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cmoulliard commented Dec 20, 2024

Uh oh!

cmoulliard commented Dec 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cmoulliard commented Dec 20, 2024

Uh oh!

nabuskey commented Dec 20, 2024

Uh oh!

punkwalker commented Dec 20, 2024

Uh oh!

cmoulliard commented Dec 23, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

punkwalker commented Dec 19, 2024 •

edited

Loading

cmoulliard Dec 20, 2024 •

edited

Loading

cmoulliard commented Dec 20, 2024 •

edited

Loading