Add support for list command to get the list of container checkpoints

[Parthiba-Hazra]

issue: #54

[github-actions[bot]]

Test Results

55 tests  +6   55 :white_check_mark: +6   1s :stopwatch: ±0s  1 suites ±0    0 :zzz: ±0   1 files   ±0    0 :x: ±0 

Results for commit 7e21d80f. ± Comparison against base commit df76f87f.

:recycle: This comment has been updated with latest results.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 87.93103% with 14 lines in your changes are missing coverage. Please review.

Project coverage is 79.15%. Comparing base (9aa0386) to head (7e21d80). Report is 24 commits behind head on main.

Files	Patch %	Lines
internal/config_extractor.go	77.08%	8 Missing and 3 partials :warning:
cmd/list.go	95.45%	2 Missing and 1 partial :warning:

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #115 +/- ## ========================================== + Coverage 78.85% 79.15% +0.29% ========================================== Files 6 11 +5 Lines 927 1276 +349 ========================================== + Hits 731 1010 +279 - Misses 154 200 +46 - Partials 42 66 +24 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[adrianreber]

Please make sure you have tests for the functionality you added.

[Parthiba-Hazra]

Please make sure you have tests for the functionality you added.

yes sure.

[adrianreber]

@Parthiba-Hazra Can you take a look add adding also an e2e test? Take a look at the make test and make test-junit.

Also, please to not include merge commits. Try to do a rebase of your PR using the latest main branch.

[Parthiba-Hazra]

@Parthiba-Hazra Can you take a look add adding also an e2e test? Take a look at the make test and make test-junit.

Also, please to not include merge commits. Try to do a rebase of your PR using the latest main branch.

@adrianreber yes I actually working on the test case and mistakenly update the PR branch .. sorry for that .

[Parthiba-Hazra]

Hey, @adrianreber @rst0git I wrote some e2e tests, right now I am not pushing those changes as there will be some conflicts against the pr #116 .

[rst0git]

I wrote some e2e tests, right now I am not pushing those changes as there will be some conflicts against the pr https://github.com/checkpoint-restore/checkpointctl/pull/116

That should be fine. We can review your changes, then resolve any merge conflicts before merging. git rebase -i HEAD~3 can be used to edit the last 3 commits in your branch, and git push with -f to update your branch in GitHub after rebase.

[Parthiba-Hazra]

@adrianreber @rst0git need a review on this pr :)

[adrianreber]

The check binary size CI run fails because one of the intermediate commits does not build. Is this one of your commits or was one of the previous commits in the repository broken?

[adrianreber]

Please also take a look at your commit messages. They do not contain much information why you made the changes you did.

[Parthiba-Hazra]

The check binary size CI run fails because one of the intermediate commits does not build. Is this one of your commits or was one of the previous commits in the repository broken?

actually that is one of previous commit . should I squash them?

[Parthiba-Hazra]

@rst0git @adrianreber I updated the PR with desired changes :)

[adrianreber]

From my point of view the commits need to be cleaned up. The commit messages seem to be the result of multiple squashes. Only one Signed-off-by is necessary and useful commit message please. Try to break lines after 72 characters.

There are multiple guides about how to write good messages. I think @rst0git always has a good link but I recently read this one (https://github.com/kubernetes/community/blob/master/contributors/guide/pull-requests.md#commit-message-guidelines) which also did not sound too bad.

In one commit you remove an unrelated test and in another commit you remove it again. That looks unnecessary. The changes also seem to be distributed in a strange way across multiple commits. It makes sense to split different changes into different commits, but not like in this PR. I think the structure of the commits needs more work at this point.

[Parthiba-Hazra]

From my point of view the commits need to be cleaned up. The commit messages seem to be the result of multiple squashes. Only one Signed-off-by is necessary and useful commit message please. Try to break lines after 72 characters.

There are multiple guides about how to write good messages. I think @rst0git always has a good link but I recently read this one (https://github.com/kubernetes/community/blob/master/contributors/guide/pull-requests.md#commit-message-guidelines) which also did not sound too bad.

In one commit you remove an unrelated test and in another commit you remove it again. That looks unnecessary. The changes also seem to be distributed in a strange way across multiple commits. It makes sense to split different changes into different commits, but not like in this PR. I think the structure of the commits needs more work at this point.

You are right currently the commits are really messed up. let me work on this.. if necessary I can create a new pr with proper commits..

[adrianreber]

You are right currently the commits are really messed up. let me work on this.. if necessary I can create a new pr with proper commits.

This is usually never necessary. With the help of git rebase and git push --force to your branch it should not be necessary to have a new PR. Also use git pull --rebase to rebase your work on the latest main branch.

[Parthiba-Hazra]

You are right currently the commits are really messed up. let me work on this.. if necessary I can create a new pr with proper commits.

This is usually never necessary. With the help of git rebase and git push --force to your branch it should not be necessary to have a new PR. Also use git pull --rebase to rebase your work on the latest main branch.

yea that's what I am gonna do. will try to fix this soon.

[Parthiba-Hazra]

@adrianreber @rst0git I hope now it satisfy the commit message guidelines . although I have no idea why there is one CI test keeps failing.

[snprajwal]

In order for us to use git bisect while debugging regressions, we require all commits to be self-contained, i.e. every commit must be able to successfully build and pass tests. It looks like your first commit has changes that are actually a part of the second commit, leading to the build breaking. That is what is causing the CI to fail - the logs indicate what the Go build errors are, and in which file.

[Parthiba-Hazra]

In order for us to use git bisect while debugging regressions, we require all commits to be self-contained, i.e. every commit must be able to successfully build and pass tests. It looks like your first commit has changes that are actually a part of the second commit, leading to the build breaking. That is what is causing the CI to fail - the logs indicate what the Go build errors are, and in which file.

That's right, I really messed up things while solving merge conflicts. Really sorry for this .

[rst0git]

That's right, I really messed up things while solving merge conflicts. Really sorry for this .

No worries, don't take code reviews as criticism; they are just advice on how your pull request can be improved ;)

[rst0git]

@adrianreber PTAL

[adrianreber]

Remove if applied from both commits. State what the commit does without any conditionals.

Also take a look at the line length there is some line breaks in the raw patch as well as in the GitHub interface.

[adrianreber]

The commit structure still needs to be reworked from my point of view.

You have two commits. First commit is:

• feat: Add support for the list command to retrieve checkpoints: This looks correct from my point of view.
• refactor: Extract necessary values from spec.dump: This is the problematic one from my point of view:
  • All changes to the list command should happen to the first commit. My point of view is, if you introduce a feature in a single PR no fixup commits should be necessary. You restructure code in the second commit. You remove complete functions (CreateTarWithFile). All that should happen in the first commit. Do not introduce something and then change it in the next commit. As long as it is not part of the main repository you can change the commit structure as much as you want. Once it has been committed and we figure out it needs a change, a commit is the right thing to do. But not in a single PR, as long as it is all contained in a PR no need for a second refactor commit. Always, just from my point of view.
  • For me, in this case it means that everything could be in one commit. I would try to separate code changes and changes to test/ in two commits, but this is something I would not insist on. It would be just nicer for a PR of this size to have separate commits for code and test changes. For a small commit it could all be in one commit.

If the refactor commit would touch other code, then that commit should be first and then your changes. But all the refactoring, as far as I can tell, is happening in the code from the previous commit, right?

As always, most things I mention are not absolute rules and it depends on the context.

[Parthiba-Hazra]

The commit structure still needs to be reworked from my point of view.

You have two commits. First commit is:

• feat: Add support for the list command to retrieve checkpoints: This looks correct from my point of view.

• refactor: Extract necessary values from spec.dump: This is the problematic one from my point of view:

  • All changes to the list command should happen to the first commit. My point of view is, if you introduce a feature in a single PR no fixup commits should be necessary. You restructure code in the second commit. You remove complete functions (CreateTarWithFile). All that should happen in the first commit. Do not introduce something and then change it in the next commit. As long as it is not part of the main repository you can change the commit structure as much as you want. Once it has been committed and we figure out it needs a change, a commit is the right thing to do. But not in a single PR, as long as it is all contained in a PR no need for a second refactor commit. Always, just from my point of view.
  • For me, in this case it means that everything could be in one commit. I would try to separate code changes and changes to test/ in two commits, but this is something I would not insist on. It would be just nicer for a PR of this size to have separate commits for code and test changes. For a small commit it could all be in one commit.

If the refactor commit would touch other code, then that commit should be first and then your changes. But all the refactoring, as far as I can tell, is happening in the code from the previous commit, right?

As always, most things I mention are not absolute rules and it depends on the context.

@adrianreber I understand. I will squash the second commit and keep the PR in one commit. Thank you for the detailed explanation; I was not really aware of good commit practices before this PR. ;)