Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

cmd/ctr: add commit command to snapshot #1160

Merged
merged 1 commit into from Jul 21, 2017
Merged

cmd/ctr: add commit command to snapshot #1160

merged 1 commit into from Jul 21, 2017

Conversation

darkowlzz
Copy link
Contributor

Signed-off-by: Sunny Gogoi me@darkowlzz.space

Part of #1082

@codecov-io
Copy link

codecov-io commented Jul 12, 2017 •
edited

Codecov Report

Merging #1160 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #1160   +/-   ##
======================================
  Coverage    28.1%   28.1%           
======================================
  Files          28      28           
  Lines        2832    2832           
======================================
  Hits          796     796           
  Misses       1887    1887           
  Partials      149     149

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 28e8d95...912ddba. Read the comment docs.

crosbymichael
crosbymichael approved these changes Jul 12, 2017
View reviewed changes
Copy link
Member

@crosbymichael crosbymichael left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

With one nit

cmd/ctr/snapshot.go Outdated
}

snapshotter := client.SnapshotService()
err = snapshotter.Commit(ctx, target, id)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: just return snapshotter.Commit(...)

@crosbymichael
Copy link
Member

LGTM

Thanks @darkowlzz

@dmcgowan
Copy link
Member

This should be rebased with the merge of multiple snapshotters
snapshotter := client.SnapshotService() now requires specifying the snapshotter. Likewise add the snapshotter options to be used here. Looks good after that.

@dmcgowan
Copy link
Member

Can you squash these commits please? The CI failure seemed to the same race, lets see if passes after squash

@dmcgowan dmcgowan added this to the containerd alpha1 milestone Jul 17, 2017
dmcgowan
dmcgowan reviewed Jul 18, 2017
View reviewed changes
cmd/ctr/snapshot.go Outdated

target := clicontext.String("target")
if target == "" {
return errors.New("target must be provided")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's not have a required flag and just have the usage be id target

@darkowlzz darkowlzz force-pushed the 1082-snapshot-commit branch 2 times, most recently from 3533958 to c9a1b7d Compare July 19, 2017 09:48
@stevvooe
Copy link
Member

Please rebase.

@darkowlzz
cmd/ctr: add commit command to snapshot
912ddba 
Signed-off-by: Sunny Gogoi <me@darkowlzz.space>
@dmcgowan
Copy link
Member

LGTM

@dmcgowan dmcgowan merged commit 0f6959a into containerd:master Jul 21, 2017
ianlewis pushed a commit to ianlewis/containerd that referenced this pull request Dec 8, 2020
@mikebrow
Merge pull request containerd#1160 from Random-Liu/remove-unused-todo
bc3b49e 
Remove an unused TODO.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dmcgowan dmcgowan dmcgowan left review comments

@crosbymichael crosbymichael crosbymichael approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
containerd beta
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@darkowlzz @codecov-io @crosbymichael @dmcgowan @stevvooe