kxr / o-must-gather

oc like tool that works with must-gather rather than OpenShift API
GNU General Public License v3.0
161 stars 73 forks source link

[feat] Add the --show-labels option for the 'omg get' output #42

Closed git-hyagi closed 3 years ago

git-hyagi commented 3 years ago

Hi @kxr,

First, I'd like to thank you for this amazing job! I use the omg every day and it is awesome :smile:

This is a proposal implementation for issue #37.

kxr commented 3 years ago

@git-hyagi Thank you for the feedback and for implementing the --show-labels feature. I will review this and merge this asap.

kxr commented 3 years ago

@git-hyagi I wanted to thank you again as I was going through the commit to merge it and realized that it was a lot of work!. A big thumbs up!

I have fixed some minor issues and improved some code from your commit. Following is the summary:

Mainly, I created a helper function extract_labels that could extract the labels from the object and return it in flat string format just like oc does.

So in the xx_out functions,

        if show_labels and "labels" in svc['metadata']:
            row.append(svc['metadata']['labels'])

became:

        if show_labels:
            row.append(extract_labels(svc))

Also this function will return '\<none>' when labels is absent in metadata, replicating what oc does.

Secondly I did some readjustment of populating the headers. For example, to accommodate the HEADER column, you changed:

    if ns == '_all':
        output_res[0].append('NAMESPACE')
    output_res[0].extend(['NAME','TYPE','CLUSTER-IP','EXTERNAL-IP','PORT(S)','AGE'])
    if output == 'wide':
        output_res[0].extend(['SELECTOR'])

To:

    if ns == '_all':
        output_res[0].append('NAMESPACE')
    if output == 'wide' and not show_labels:
        output_res[0].extend(['NAME','TYPE','CLUSTER-IP','EXTERNAL-IP','PORT(S)','AGE','SELECTOR'])
    elif output == 'wide' and show_labels:
        output_res[0].extend(['NAME','TYPE','CLUSTER-IP','EXTERNAL-IP','PORT(S)','AGE','SELECTOR','LABELS'])
    elif show_labels:
        output_res[0].extend(['NAME','TYPE','CLUSTER-IP','EXTERNAL-IP','PORT(S)','AGE','LABELS'])
    else:
        output_res[0].extend(['NAME','TYPE','CLUSTER-IP','EXTERNAL-IP','PORT(S)','AGE'])

Which works, but I found a more intuitive way:

    if ns == '_all':
        output_res[0].append('NAMESPACE')
    output_res[0].extend(['NAME','TYPE','CLUSTER-IP','EXTERNAL-IP','PORT(S)','AGE'])
    if output == 'wide':
        output_res[0].extend(['SELECTOR'])
    if show_labels:
        output_res[0].extend(['LABELS'])

Best Regards, Khizer

git-hyagi commented 3 years ago

@kxr I'm glad to help :+1: Thank you for keep doing the amazing job in the o-must-gather and for the feedback on the code improvement!! :smile: