Closed Parthiba-Hazra closed 5 months ago
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 ajson.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?
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.
@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.
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.
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?
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
pick
to s
or squash
in the rebase windowpick
and squash
commits)git push --force-with-lease
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
tos
orsquash
in the rebase window- Ensure the new commit contains the correct message (by default, git combines the messages of the
pick
andsquash
commits)git push --force-with-lease
thanks prajwal for the guidance here, really forgot about squash.
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 :warning: |
checkpointctl.go | 0.00% | 1 Missing :warning: |
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
48 tests ±0 48 :white_check_mark: ±0 1s :stopwatch: ±0s 1 suites ±0 0 :zzz: ±0 1 files ±0 0 :x: ±0
Results for commit 20d7bfe3. ± Comparison against base commit 9aa0386e.
:recycle: This comment has been updated with latest results.
@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.
issue: #77