Refactor `checkpointctl.go` and move commands into individual files about checkpointctl HOT 16 CLOSED

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 :)

from checkpointctl.

@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.

from checkpointctl.

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.

from checkpointctl.

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

from checkpointctl.

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

from checkpointctl.

@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?

from checkpointctl.

@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.

from checkpointctl.

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.

from checkpointctl.

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.

from checkpointctl.

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.

from checkpointctl.

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

from checkpointctl.

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.

from checkpointctl.

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.

from checkpointctl.

@snprajwal @adrianreber

from checkpointctl.

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

from checkpointctl.

@snprajwal Opened a draft PR.

from checkpointctl.