Skip to content
This repository has been archived by the owner on Dec 7, 2023. It is now read-only.

Object inspection support #107

Merged
merged 2 commits into from
Jul 8, 2019
Merged

Object inspection support #107

merged 2 commits into from
Jul 8, 2019

Conversation

twelho
Copy link
Contributor

@twelho twelho commented Jul 8, 2019

This PR adds the ignite inspect command to retrieve the information/configuration of an Ignite object.

Copy link
Contributor

@luxas luxas left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM 👍
some nits

}

func addInspectFlags(fs *pflag.FlagSet, i *run.InspectFlags) {
fs.BoolVarP(&i.YAMLOutput, "yaml", "y", false, "Output the object in YAML format instead of JSON")
Copy link
Contributor

Choose a reason for hiding this comment

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

doesn't have to be done now, but eventually this should be done as --output=<type> => -o=yaml, -o=json instead for reusability and consistency.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reworked in c6622cf.

"github.com/spf13/pflag"
"github.com/weaveworks/ignite/cmd/ignite/run"
"github.com/weaveworks/ignite/pkg/errutils"
"io"
Copy link
Contributor

Choose a reason for hiding this comment

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

should be in its own group up top

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in c6622cf.

"fmt"
"github.com/weaveworks/ignite/pkg/apis/ignite/scheme"
"github.com/weaveworks/ignite/pkg/client"
"github.com/weaveworks/ignite/pkg/filter"
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: grouping

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in c6622cf.

var kind meta.Kind
io := &inspectOptions{InspectFlags: i}

switch k {
Copy link
Contributor

Choose a reason for hiding this comment

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

strings.ToLower(k) to support ignite inspect Image foo if somebody would do it. no need to error in that case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, was not sure if it's needed as it could train the user "incorrect" usage. Implemented in c6622cf now.

Run `make tidy` as well which fixes the import ordering.
Copy link
Contributor

@luxas luxas left a comment

Choose a reason for hiding this comment

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

LGTM

@luxas luxas merged commit ab2eba6 into master Jul 8, 2019
@luxas luxas deleted the inspect branch July 8, 2019 17:43
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants