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

Add '-group-by' flag to compute aggregated disk usage #48

Merged
merged 3 commits into from
Apr 19, 2023

Conversation

fedordikarev
Copy link
Contributor

@fedordikarev fedordikarev commented Apr 12, 2023

Draft on how "Group by" functionality could be added.

Example output:

> unused -gcp.project dev-project -group-by kubernetes.io/created-for/pvc/namespace
PROVIDER  GROUP_BY                         TYPE    SIZE_GB
GCP       agent-smoke-test                 ssd     30
GCP       default                          hdd     941
GCP       preview                          ssd     120
GCP       NONE                             ssd     1000
GCP       ge-logs                          hdd     5
...

> unused -gcp.project dev-project -group-by zone
PROVIDER  GROUP_BY       TYPE    SIZE_GB
GCP       us-central1-a  ssd     56370
GCP       us-central1-a  hdd     4340
GCP       us-central1-b  ssd     10528
GCP       us-central1-f  ssd     6188
GCP       us-central1-f  hdd     451
GCP       us-central1-b  hdd     1150

@fedordikarev fedordikarev linked an issue Apr 12, 2023 that may be closed by this pull request
Copy link
Collaborator

@inkel inkel left a comment

Choose a reason for hiding this comment

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

I like the idea, and the implementation looks good. Can you past the output of the table in the PR description?

@fedordikarev fedordikarev marked this pull request as ready for review April 14, 2023 20:01
@fedordikarev fedordikarev requested a review from inkel April 17, 2023 16:33
Copy link
Collaborator

@inkel inkel left a comment

Choose a reason for hiding this comment

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

Thanks for the example! It made it far easier to understand the changes. I've left some comments, tho, which I'd love to discuss. I created a different thread for each one so it's easier to discuss.


w := tabwriter.NewWriter(os.Stdout, 8, 4, 2, ' ', 0)

headers := []string{"PROVIDER", "GROUP_BY", "TYPE", "SIZE_GB"}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure GROUP_BY is clear enough as a column header. I'd love to have the value passed by the user for the -group-by flag, although I'm not sure how would that look in the case of something like kubernetes.io/created-for/pvc/namespace for instance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have doubts here as well and can't find good enough solution for column name.
Tag names are long and also confusing when I tried to use them.

I have 3 ideas:

  • add comment about grouping tag when run with -v flag and use GROUP_BY_TAG as column name
    Grouping by 'kubernetes.io/created-for/pvc/namespace' tag.
    PROVIDER  GROUP_BY_TAG    TYPE    DISKS_COUNT  TOTAL_SIZE_GB
    GCP       redis-test      ssd     3            9
    
  • print group-by name as column
    PROVIDER  kubernetes.io/created-for/pvc/namespace  TYPE    DISKS_COUNT  TOTAL_SIZE_GB
    GCP       redis-test                               ssd     3            9
    
  • print upcased group-by name
    PROVIDER  KUBERNETES.IO/CREATED-FOR/PVC/NAMESPACE  TYPE    DISKS_COUNT  TOTAL_SIZE_GB
    GCP       redis-test                               ssd     3            9
    

what do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think I like the second option the better. It's definitely a hard question to answer 🤔

cmd/unused/internal/ui/group_table.go Outdated Show resolved Hide resolved
cmd/unused/internal/ui/group_table.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@inkel inkel left a comment

Choose a reason for hiding this comment

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

Left a few more comments, mostly aesthetic.

cmd/unused/internal/ui/group_table.go Outdated Show resolved Hide resolved
cmd/unused/internal/ui/group_table.go Outdated Show resolved Hide resolved
cmd/unused/internal/ui/group_table.go Outdated Show resolved Hide resolved
cmd/unused/internal/ui/group_table.go Outdated Show resolved Hide resolved
cmd/unused/internal/ui/group_table.go Outdated Show resolved Hide resolved
@fedordikarev
Copy link
Contributor Author

thanks @inkel , addressed all your comments :)

@fedordikarev fedordikarev requested a review from inkel April 19, 2023 15:34
Copy link
Collaborator

@inkel inkel left a comment

Choose a reason for hiding this comment

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

Love it! Thanks!

@fedordikarev fedordikarev merged commit 5fed19a into main Apr 19, 2023
1 check passed
@fedordikarev fedordikarev deleted the fedor/group_by branch April 19, 2023 17:37
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.

Aggregate disk usage by values from metadata
2 participants