Extend inspect output format with json

[Parthiba-Hazra]

issue: #77

[behouba]

Hi @Parthiba-Hazra, thank you for taking the time to work on this issue. I have added some comments.

[snprajwal]

This looks good for the most part, but I'm not too convinced about using a map[string]interface{} for storing the JSON object. interface{} adds a runtime cost to ensure type safety, and might end up having significant overhead for bigger checkpoints. I think marshaling the data in chunks into a json.RawMessage object and doing some string slicing will be considerably more efficient. There's something similar implemented in https://github.com/checkpoint-restore/go-criu/blob/master/crit/image.go#L85-L97, maybe you can check it out and see if that can be used here.

[Parthiba-Hazra]

This looks good for the most part, but I'm not too convinced about using a map[string]interface{} for storing the JSON object. interface{} adds a runtime cost to ensure type safety, and might end up having significant overhead for bigger checkpoints. I think marshaling the data in chunks into a json.RawMessage object and doing some string slicing will be considerably more efficient. There's something similar implemented in https://github.com/checkpoint-restore/go-criu/blob/master/crit/image.go#L85-L97, maybe you can check it out and see if that can be used here.

yes you are right, should I go for custom struct instead of interfaces?

[snprajwal]

yes you are right, should I go for custom struct instead of interfaces?

Yes, that would be ideal! The fields that are not populated can be omitted from the final JSON output by marking them with the omitempty attribute.

[behouba]

@Parthiba-Hazra, let's not add fix-up commits. Instead, amend the initial commit or add new self contained commits. This will ensure that that the commit history is clean. Checkout the CRIU contribution guidelines for more details.
It's also a good idea to test the JSON format.

[Parthiba-Hazra]

@Parthiba-Hazra, let's not add fix-up commits. Instead, amend the initial commit or make add new self contained commits. This will ensure that that the commit history is clean. Checkout the CRIU contribution guidelines for more details. It's also a good idea to test the JSON format.

sorry if I am wrong but I didn't perform fixup commits, I just added new commits on top of other commits.

[adrianreber]

The second commit looks indeed like a fixup. You introduce a new feature in the first commit and then you say "minor changes" in the second commit. If you introduce a new feature in one PR you can separate logical changes in different commits but fixups like "minor changes" should be part of the first commit. No need to create a second commit. You can include the fixup in the original commit as long as it is a PR and not yet merged. Once it is merged and it needs to change the situation is different. A commit message like "minor changes" would probably also be questionable and would require a better description of the change.

[Parthiba-Hazra]

The second commit looks indeed like a fixup. You introduce a new feature in the first commit and then you say "minor changes" in the second commit. If you introduce a new feature in one PR you can separate logical changes in different commits but fixups like "minor changes" should be part of the first commit. No need to create a second commit. You can include the fixup in the original commit as long as it is a PR and not yet merged. Once it is merged and it needs to change the situation is different. A commit message like "minor changes" would probably also be questionable and would require a better description of the change.

yea got it, sorry for the inconvenience, I just got to know about this fixup commit thing, will keep in my mind for sure. should I create another pr with proper commits?

[snprajwal]

should I create another pr with proper commits?

Not necessary, you can rebase your branch to fix this. In your case, you need to squash your two recent commits into the oldest commit (extend inspect output format with json). You can do this with the below steps:

• git rebase -i HEAD~3
• Change the latest two commits from pick to s or squash in the rebase window
• Ensure the new commit contains the correct message (by default, git combines the messages of the pick and squash commits)
• git push --force-with-lease

[Parthiba-Hazra]

should I create another pr with proper commits?

Not necessary, you can rebase your branch to fix this. In your case, you need to squash your two recent commits into the oldest commit (extend inspect output format with json). You can do this with the below steps:

• git rebase -i HEAD~3
• Change the latest two commits from pick to s or squash in the rebase window
• Ensure the new commit contains the correct message (by default, git combines the messages of the pick and squash commits)
• git push --force-with-lease

thanks prajwal for the guidance here, really forgot about squash.

[codecov-commenter]

Codecov Report

Attention: 235 lines in your changes are missing coverage. Please review.

Comparison is base (9aa0386) 78.85% compared to head (20d7bfe) 62.96%.

Files	Patch %	Lines
json.go	0.00%	234 Missing ⚠️
checkpointctl.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             main     #113       +/-   ##
===========================================
- Coverage   78.85%   62.96%   -15.90%     
===========================================
  Files           6        7        +1     
  Lines         927     1161      +234     
===========================================
  Hits          731      731               
- Misses        154      388      +234     
  Partials       42       42               

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[github-actions]

Test Results

48 tests  ±0   48 ✅ ±0   1s ⏱️ ±0s
 1 suites ±0    0 💤 ±0 
 1 files   ±0    0 ❌ ±0 

Results for commit 20d7bfe. ± Comparison against base commit 9aa0386.

♻️ This comment has been updated with latest results.

[rst0git]

@Parthiba-Hazra nice work!

[adrianreber]

@Parthiba-Hazra Please take a look at the failed CI results. Also take a look at adding tests. Currently codecov says 0% patch test coverage.