feat: add edit subcommand#205
Conversation
Binary Size Check FailedThe binary size increase exceeds the allowed threshold. Size Check OutputHow to resolveIf this size increase is expected and acceptable, a maintainer can add Binary size check performed by CI |
|
I have tested the functionality on Podman and CRI-O checkpoints. I will start working on writing the tests and would appreciate any feedback and guidance. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #205 +/- ##
==========================================
- Coverage 79.98% 77.84% -2.15%
==========================================
Files 16 19 +3
Lines 1759 2040 +281
==========================================
+ Hits 1407 1588 +181
- Misses 263 328 +65
- Partials 89 124 +35 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@rst0git, please take a look |
|
Please take a look at providing unit tests and end to end tests. |
ImranullahKhann
force-pushed
the
main
branch
2 times, most recently
from
e536151 to
1c8ceeb
Compare
|
@adrianreber, please have a look |
|
Try to add more unit tests. Especially the error branches are not covered well. |
Currently only supports tcp listen port remapping. Made the following changes: - Added the edit command in cmd/. - Added edit_archive.go in internals/ for the main edit logic. - Added archive_modifiers.go inside internals/. It contains the modifier logic for any entry that would need changes in the archive. Kept extensibility in mind, for any additional change logic in the future, we can reuse the same function tarStreamRewrite (in edit_archive.go) and write additional modifier functions in archive_modifiers.go. Assisted-by: Github Copilot Signed-off-by: Imranullah Khan <imranullahkhann2004@gmail.com>
Signed-off-by: Imranullah Khan <imranullahkhann2004@gmail.com>
|
Please have a look |
There was a problem hiding this comment.
@ImranullahKhann thank you for working on this. I added some comments, feel free to take a look when you can.
| continue | ||
| } | ||
|
|
||
| containerPort, ok := mappingMap["container_port"] |
There was a problem hiding this comment.
If i understand correctly, this match on the container port only, so --tcp-listen-remap could rewrite UDP mapping that share the same port (common situation with containers like pihole and coredns). Would it make sense to skip non TCP ports here?
if proto, _ := mappingMap["protocol"].(string); proto != "" && proto != "tcp" {
continue
}| b := make([]byte, 10) | ||
| n, err := io.ReadFull(archiveFile, b) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to read archive magic bytes") |
There was a problem hiding this comment.
| return fmt.Errorf("failed to read archive magic bytes") | |
| return fmt.Errorf("failed to read archive magic bytes: %w", err) |
| comp := archive.DetectCompression(b[:n]) | ||
| // Seek back so DecompressStream can read from the start | ||
| if _, err := archiveFile.Seek(0, 0); err != nil { | ||
| return fmt.Errorf("failed to seek archive") |
There was a problem hiding this comment.
| return fmt.Errorf("failed to seek archive") | |
| return fmt.Errorf("failed to seek archive: %w", err) |
| // Decompress the archive into a plan tar stream | ||
| tarStream, err := archive.DecompressStream(archiveFile) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to decompress archive") |
There was a problem hiding this comment.
| return fmt.Errorf("failed to decompress archive") | |
| return fmt.Errorf("failed to decompress archive: %w", err) |
| if err != nil { | ||
| outFile.Close() | ||
| os.Remove(outputPath) | ||
| return fmt.Errorf("failed to create compressor") |
There was a problem hiding this comment.
| return fmt.Errorf("failed to create compressor") | |
| return fmt.Errorf("failed to create compressor: %w", err) |
| var config map[string]any | ||
| if err := json.Unmarshal(data, &config); err != nil { | ||
| return nil, nil, fmt.Errorf("parsing config.dump JSON: %w", err) | ||
| } |
There was a problem hiding this comment.
Big integers get rounded here because of map[string]any will decode all numbers as float64. This can corrupt the output when the default system resource limits are overwritten ( --ulimit).
Maybe something like this can keep the fidelity is unchanged.
dec := json.NewDecoder(bytes.NewReader(data))
dec.UseNumber()
var config map[string]any
if err := dec.Decode(&config); err != nil {
return nil, nil, fmt.Errorf("parsing config.dump JSON: %w", err)
}| var spec map[string]any | ||
| if err := json.Unmarshal(data, &spec); err != nil { | ||
| return nil, nil, fmt.Errorf("parsing spec.dump JSON: %w", err) | ||
| } |
There was a problem hiding this comment.
same possible issue as above.
4 participants
Currently only supports tcp listen port remapping. Made the following changes:
Kept extensibility in mind, for any additional edit logic in the future, we can reuse the same function tarStreamRewrite (in edit_archive.go) and write additional modifier functions in archive_modifiers.go.
Closes #169