Refactor `checkpointctl.go` and move commands into individual files

[snprajwal]

The checkpointctl.go file has grown after adding the new inspect command, and will grow further as we expand the CLI. It would be better to refactor it into it's own package, and use the existing file only to invoke the main function.

[shikharish]

Hello @snprajwal, what is the status of this issue? Can I work on this?

[snprajwal]

This is not actively being worked on - do feel free to take it up!

[shikharish]

@snprajwal Thanks! Pls assign me this. Also, can you pls explain the issue a bit more. What things should I keep in mind while refactoring?

[snprajwal]

@shikharish it would mainly require creating a cmd package and moving the commands into individual files under that package, i.e. the show() and setupShow() functions would go in show.go, similarly with inspect.go, and the remaining functions under util.go. This package can be imported in the existing checkpointctl.go file and used in the main function.

[shikharish]

@snprajwal Should I also move container.go and memparse.go to cmd pacakge as they will be used in show.go, inspect.go and utils.go.

[snprajwal]

IMO, it would be better to leave those files as is for now. If necessary, they can be split up in a separate PR going forward.

[shikharish]

IMO, it would be better to leave those files as is for now. If necessary, they can be split up in a separate PR going forward.

@snprajwal I mean I can't use the functions from controller.go and memparse.go(which are in the main package) in cmd package. Because we cannot import the main package as far as I know.

[snprajwal]

I can't use the functions from controller.go and memparse.go(which are in the main package) in cmd package.

Ah, I assumed you were asking about whether those files should be split up into smaller components. Yes, the files should be moved, but not into cmd. Placing them in the existing lib package is more appropriate.

[adrianreber]

About lib/. That is currently included in a couple of projects and basically has no external dependencies. This is a bit unfortunate that the lib/ namespace is blocked, but I just wanted to highlight that it needs to be in a different directory so that projects including lib/metadata.go do not get additional dependencies pulled in.

[snprajwal]

In that case, how about internal? These files shouldn't be accessed by an external entity as a dependency anyway, it's purely for use within checkpointctl

[adrianreber]

In that case, how about internal? These files shouldn't be accessed by an external entity as a dependency anyway, it's purely for use within checkpointctl

That should work. internal/ is a special name if I remember correctly and cannot be included in other projects, right? But if that will be a problem it can be renamed.

[shikharish]

Hello, am working on this. It is a bit more complicated then I expected it to be. A lot of files have to moved and changed. Would need some more time. This is the directory structure right now:

cmd
├── inspect.go
├── memparse.go
└── show.go

internal
├── container.go
├── tree.go
├── utils.go
└── utils_test.go

lib
└── metadata.go

checkpointctl.go

Please review whether this is appropriate.

[shikharish]

@snprajwal @adrianreber

[snprajwal]

It would be hard to give a yes or no depending on just the directory structure - the contents of the files matters too. I would suggest opening a draft PR with the work, and we can iterate and make the appropriate changes :)

[adrianreber]

I agree. Open a PR and let's discuss your work there. Having the actual changes makes the discussion much easier.

[shikharish]

@snprajwal Opened a draft PR.