feature: add a paramter labels to CreateSnapshot #2569
Conversation
Signed-off-by: yifang.jy <yifang.jy@alibaba-inc.com>
Codecov Report
@@ Coverage Diff @@
## master #2569 +/- ##
=========================================
Coverage ? 69.17%
=========================================
Files ? 278
Lines ? 18687
Branches ? 0
=========================================
Hits ? 12926
Misses ? 4284
Partials ? 1477
|
|
We found this is your first time to contribute to Pouch, @firjest |
|
yifang.jy seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
ctrd/interface.go
Outdated
| @@ -93,7 +93,7 @@ type ImageAPIClient interface { | |||
| // SnapshotAPIClient provides access to containerd snapshot features | |||
| type SnapshotAPIClient interface { | |||
| // CreateSnapshot creates a active snapshot with image's name and id. | |||
| CreateSnapshot(ctx context.Context, id, ref string) error | |||
| CreateSnapshot(ctx context.Context, id, ref string, labels *map[string]string) error | |||
There was a problem hiding this comment.
Why we should use *map[string]string
ctrd/snapshot.go
Outdated
| @@ -31,8 +31,13 @@ func (c *Client) CreateSnapshot(ctx context.Context, id, ref string) error { | |||
| return err | |||
| } | |||
|
|
|||
| var opts []snapshots.Opt | |||
| if labels != nil && len(*labels) > 0 { | |||
| opts = append(opts, snapshots.WithLabels(*labels)) | |||
There was a problem hiding this comment.
How about just opts := []snapshots.Opt{snapshots.WithLabels(labels)}, of course, not use *map here, see last comment
| @@ -383,7 +383,7 @@ func (mgr *ContainerManager) Create(ctx context.Context, name string, config *ty | |||
|
|
|||
| snapID := id | |||
| // create a snapshot with image. | |||
| if err := mgr.Client.CreateSnapshot(ctx, snapID, config.Image); err != nil { | |||
| if err := mgr.Client.CreateSnapshot(ctx, snapID, config.Image, nil); err != nil { | |||
There was a problem hiding this comment.
Seems we should get somewhere to pass the label, or it will always be nil
There was a problem hiding this comment.
We can add a snapshot-opt which keep options for snapshotter
|
To support this feature, containerd should make some changes like this! https://github.com/alibaba/containerd/pull/5/files#diff-491f07c28eb851b720f11ce0d3dbb4f8R331 |
|
@firjest Thanks for your contribution. 🍻 |
|
@wangforthinker , we should consider do not effect containerd, let's hold on this pr |
allencloud
changed the title
|
ping @firjest |
Signed-off-by: yifang.jy yifang.jy@alibaba-inc.com
Ⅰ. Describe what this PR did
snapshot interface has labels parameter, so add this parameter to pouch CreateSnapshot In some case use can pass labels.
Ⅱ. Does this pull request fix one issue?
NO
Ⅲ. Why don't you add test cases (unit test/integration test)? (你真的觉得不需要加测试吗？)
NO
Ⅳ. Describe how to verify it
NO
Ⅴ. Special notes for reviews