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

rename snapshot->snapshots packages, add aliases #1797

Merged
merged 3 commits into from
Nov 30, 2017
Merged

rename snapshot->snapshots packages, add aliases #1797

merged 3 commits into from
Nov 30, 2017

Conversation

jessvalarezo
Copy link
Contributor

@jessvalarezo jessvalarezo commented Nov 22, 2017 •
edited
Loading

moves snapshot to plural. an effort to keep packages consistent.

  1. rename snapshot -> snapshots

  2. rename api/services/snapshot/v1 -> api/services/snapshots/v1

  3. rename services/snapshot -> services/snapshots

  4. add aliases to ctr commands singular and plural can be used

addresses #1753

@codecov-io
Copy link

codecov-io commented Nov 22, 2017 •
edited
Loading

Codecov Report

Merging #1797 into master will increase coverage by 4.92%.
The diff coverage is 71.53%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1797      +/-   ##
==========================================
+ Coverage   44.25%   49.17%   +4.92%     
==========================================
  Files          67       86      +19     
  Lines        7104     8518    +1414     
==========================================
+ Hits         3144     4189    +1045     
- Misses       3467     3659     +192     
- Partials      493      670     +177
Flag Coverage Δ
#linux 49.27% <71.53%> (?)
#windows 44.25% <42.16%> (ø) ⬆️
Impacted Files Coverage Δ
snapshots/storage/metastore.go 51.72% <ø> (ø)
server/server.go 2.15% <0%> (ø) ⬆️
metadata/db.go 66.93% <100%> (ø) ⬆️
snapshots/overlay/overlay.go 48.89% <60%> (ø)
snapshots/naive/naive.go 47.96% <60%> (ø)
metadata/snapshot.go 50.27% <72%> (+28.72%) ⬆️
snapshots/btrfs/btrfs.go 52.19% <81.81%> (ø)
snapshots/storage/bolt.go 61.89% <89.28%> (ø)
oci/spec_opts.go 0% <0%> (ø)
fs/diff_unix.go 51.61% <0%> (ø)
... and 26 more

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 59bd196...1dd6f33. Read the comment docs.

stevvooe
stevvooe reviewed Nov 22, 2017
View reviewed changes
metadata/gc.go Outdated
@@ -26,7 +26,7 @@ const (

var (
labelGCRoot = []byte("containerd.io/gc.root")
labelGCSnapRef = []byte("containerd.io/gc.ref.snapshot.")
labelGCSnapRef = []byte("containerd.io/gc.ref.snapshots.")
Copy link
Member

Choose a reason for hiding this comment

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

This is a breaking change. Let's make sure this gets added to the release notes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a good target for migration? Would be a nice way to test that functionality

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i can keep it as snapshot

Copy link
Member

Choose a reason for hiding this comment

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

This should stay as singular, since it is dealing with a singular thing.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with Stephen, only 1 snapshot is referenced

@stevvooe
Copy link
Member

LGTM

@stevvooe stevvooe added this to the 1.0.0 milestone Nov 23, 2017
@AkihiroSuda
Copy link
Member

moves snapshot to plural. an effort to keep packages consistent.

Can we consider pluralizing archive, content, design, dialer, diff, mount, plugin, reaper, reference, runtime, and server as well ? (or singularizing everything)
#1753 (comment)

@dnephin
Copy link
Contributor

dnephin commented Nov 23, 2017

Most package names (including the stdlib) are singular. I think it would be confusing to make them all plural.

@jessvalarezo
Copy link
Contributor Author

I am looking to make the ctr commands plural when possible, as well as the services they map to. I vote for keeping everything else singular.

@stevvooe
Copy link
Member

@dnephin @AkihiroSuda Some things are plural, some things are singular, such is the way of the universe. Snapshotters, defined in the snapshots package deal with snapshots, which is plural.

@jessvalarezo Please rebase so we can merge this.

@jessvalarezo
ctr: snapshot->snapshots cmd, add aliases
61c8fe2 
Signed-off-by: Jess Valarezo <valarezo.jessica@gmail.com>
@jessvalarezo
rename snapshot->snapshots pkg
9885edf 
Signed-off-by: Jess Valarezo <valarezo.jessica@gmail.com>
@jessvalarezo
rename snapshot->snapshots proto pkg and service
1dd6f33 
Signed-off-by: Jess Valarezo <valarezo.jessica@gmail.com>
@stevvooe
Copy link
Member

LGTM

1 similar comment
@dmcgowan
Copy link
Member

LGTM

@dmcgowan dmcgowan merged commit 96ca973 into containerd:master Nov 30, 2017
This was referenced Nov 30, 2017
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stevvooe stevvooe stevvooe left review comments

@dmcgowan dmcgowan dmcgowan left review comments

@mlaventure mlaventure mlaventure left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.0.0
Development

Successfully merging this pull request may close these issues.
7 participants
@jessvalarezo @codecov-io @stevvooe @AkihiroSuda @dnephin @dmcgowan @mlaventure