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(resource-labels): Add labels flag #637

Merged
merged 1 commit into from
Oct 16, 2024
Merged

feat(resource-labels): Add labels flag #637

merged 1 commit into from
Oct 16, 2024

Conversation

femrtnz
Copy link
Collaborator

@femrtnz femrtnz commented Sep 27, 2024

As part of the issue #405 and and #410 , we are adding a new flag called --labels.
This flag is not active as default

  • to avoid any braking changes.
  • it might be too noise for some people as some resources contain several labels.

Setting the flag --labels=true will include in the output a list of labels for each resource, please check the example output below

Example for TEXT output

__________________________________________________________________________________________
>>> Deprecated APIs removed in 1.26 <<<
------------------------------------------------------------------------------------------
KIND                      NAMESPACE     NAME            API_VERSION           REPLACE_WITH      (SINCE)     LABELS
HorizontalPodAutoscaler   <undefined>   test-labels-1   autoscaling/v2beta2   autoscaling/v2    (1.23.0)    k8s-app:gcp-compute-persistent-disk-csi-driver

Example for CSV output

api_version,kind,namespace,name,replace_with,since,rule_set,labels
autoscaling/v2beta2,HorizontalPodAutoscaler,<undefined>,test-labels-1,autoscaling/v2,1.23.0,Deprecated APIs removed in 1.26,k8s-app:gcp-compute-persistent-disk-csi-driver

Example for JSON output

[
	{
		"Name": "test-labels-1",
		"Namespace": "\u003cundefined\u003e",
		"Kind": "HorizontalPodAutoscaler",
		"ApiVersion": "autoscaling/v2beta2",
		"RuleSet": "Deprecated APIs removed in 1.26",
		"ReplaceWith": "autoscaling/v2",
		"Since": "1.23.0",
		"Labels": {
			"k8s-app": "gcp-compute-persistent-disk-csi-driver"
		}
	}
]

@femrtnz femrtnz force-pushed the feat/add-labels branch 3 times, most recently from 9b3ce13 to d457ab9 Compare September 29, 2024 19:31
@femrtnz
Copy link
Collaborator Author

femrtnz commented Sep 29, 2024

Hey @dark0dave I appreciate any insights on this change 🙏

Copy link

@eyalzek eyalzek left a comment

Choose a reason for hiding this comment

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

Left some comments as I'm not familiar enough with the project nor the style guide :)

pkg/config/config_test.go Show resolved Hide resolved
pkg/printer/csv.go Show resolved Hide resolved
pkg/printer/printer.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@migueldelucasdoit migueldelucasdoit left a comment

Choose a reason for hiding this comment

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

Hey @femrtnz can we have a look at the duplicated code we introduced in this PR?
https://github.com/doitintl/kube-no-trouble/pull/637/checks?check_run_id=30865045590

We can refactor the code to extract an internal function with the duplicated code.
https://www.jetbrains.com/help/go/extract-method.html

@dark0dave
Copy link
Collaborator

This PR is too long, style is incorrect and introduces way too many commits. Rebase and clean this up.

Copy link
Collaborator

@dark0dave dark0dave left a comment

Choose a reason for hiding this comment

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

Clean this up. Or split into seperate PRS.

@dark0dave
Copy link
Collaborator

dark0dave commented Oct 1, 2024

Printer should not depend on config. Please don't create this binding....

@femrtnz
Copy link
Collaborator Author

femrtnz commented Oct 1, 2024

Judge should not depend on config. Please don't create this binding....

Could you please detail what you mean? which file, line etc? it will be easier if you comment on the file.

@dark0dave
Copy link
Collaborator

Also why has the printer changed.... Like this feels brittle.

@femrtnz femrtnz force-pushed the feat/add-labels branch 3 times, most recently from 5908968 to f077618 Compare October 8, 2024 11:23
@femrtnz femrtnz requested a review from dark0dave October 8, 2024 13:54
Copy link
Collaborator

@migueldelucasdoit migueldelucasdoit left a comment

Choose a reason for hiding this comment

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

Just some typos to fix and some recommendations.

pkg/printer/csv.go Outdated Show resolved Hide resolved
pkg/context/context-keys.go Outdated Show resolved Hide resolved
pkg/printer/csv_test.go Outdated Show resolved Hide resolved
pkg/printer/printer_helper_test.go Outdated Show resolved Hide resolved
pkg/printer/text.go Outdated Show resolved Hide resolved
@femrtnz femrtnz force-pushed the feat/add-labels branch 2 times, most recently from f72aae0 to c6f59f0 Compare October 10, 2024 08:46
@femrtnz femrtnz merged commit e919a04 into master Oct 16, 2024
23 checks passed
@femrtnz femrtnz deleted the feat/add-labels branch October 16, 2024 08:20
@dark0dave
Copy link
Collaborator

Printer should not depend on config. Please don't create this binding....

@femrtnz
Read this again and now understand my pr #649

@dark0dave
Copy link
Collaborator

This pr is massive mistake. Not the functionality the way it was done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants