Skip to content
This repository was archived by the owner on Aug 14, 2020. It is now read-only.

schema/types: rework empty volume type code #541

Merged
jonboulle merged 4 commits into from
Nov 26, 2015
Merged

schema/types: rework empty volume type code #541

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c9c118c
schema/types: rework empty volume type code
jonboulle Nov 26, 2015
e151013
schema/types: add Volume.String() benchmark
jonboulle Nov 26, 2015
1141f80
schema/types: construct string more efficiently
jonboulle Nov 26, 2015
b2aaa28
schema/types: use *bool helper func for consistency
jonboulle Nov 26, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
109 changes: 77 additions & 32 deletions schema/types/volume.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,17 @@ import (
"net/url"
"path/filepath"
"strconv"
"strings"

"github.com/appc/spec/schema/common"
)

const (
emptyVolumeDefaultMode = "0755"
emptyVolumeDefaultUID = 0
emptyVolumeDefaultGID = 0
)

// Volume encapsulates a volume which should be mounted into the filesystem
// of all apps in a PodManifest
type Volume struct {
Expand All @@ -37,9 +44,9 @@ type Volume struct {
ReadOnly *bool `json:"readOnly,omitempty"`

// currently used only by "empty"
Mode string `json:"mode,omitempty"`
UID int `json:"uid,omitempty"`
GID int `json:"gid,omitempty"`
Mode *string `json:"mode,omitempty"`
UID *int `json:"uid,omitempty"`
GID *int `json:"gid,omitempty"`
}

type volume Volume
Expand All @@ -54,20 +61,29 @@ func (v Volume) assertValid() error {
if v.Source != "" {
return errors.New("source for empty volume must be empty")
}
if v.Mode == "" {
if v.Mode == nil {
return errors.New("mode for empty volume must be set")
}
if v.UID == -1 {
if v.UID == nil {
return errors.New("uid for empty volume must be set")
}
if v.GID == -1 {
if v.GID == nil {
return errors.New("gid for empty volume must be set")
}
return nil
case "host":
if v.Source == "" {
return errors.New("source for host volume cannot be empty")
}
if v.Mode != nil {
return errors.New("mode for host volume cannot be set")
}
if v.UID != nil {
return errors.New("uid for host volume cannot be set")
}
if v.GID != nil {
return errors.New("gid for host volume cannot be set")
}
if !filepath.IsAbs(v.Source) {
return errors.New("source for host volume must be absolute path")
}
Expand All @@ -79,21 +95,14 @@ func (v Volume) assertValid() error {

func (v *Volume) UnmarshalJSON(data []byte) error {
var vv volume
vv.Mode = "0755"
vv.UID = 0
vv.GID = 0
if err := json.Unmarshal(data, &vv); err != nil {
return err
}
nv := Volume(vv)
maybeSetDefaults(&nv)
if err := nv.assertValid(); err != nil {
return err
}
if nv.Kind != "empty" {
nv.Mode = ""
nv.UID = -1
nv.GID = -1
}
*v = nv
return nil
}
Expand All @@ -106,26 +115,43 @@ func (v Volume) MarshalJSON() ([]byte, error) {
}

func (v Volume) String() string {
s := fmt.Sprintf("%s,kind=%s,readOnly=%t", v.Name, v.Kind, *v.ReadOnly)
s := []string{
v.Name.String(),
",kind=",
v.Kind,
}
if v.Source != "" {
s = s + fmt.Sprintf(",source=%s", v.Source)
s = append(s, ",source=")
s = append(s, v.Source)
}
if v.Mode != "" && v.UID != -1 && v.GID != -1 {
s = s + fmt.Sprintf(",mode=%s,uid=%d,gid=%d", v.Mode, v.UID, v.GID)
if v.ReadOnly != nil {
s = append(s, ",readOnly=")
s = append(s, strconv.FormatBool(*v.ReadOnly))
}
return s
switch v.Kind {
case "empty":
if *v.Mode != emptyVolumeDefaultMode {
s = append(s, ",mode=")
s = append(s, *v.Mode)
}
if *v.UID != emptyVolumeDefaultUID {
s = append(s, ",uid=")
s = append(s, strconv.Itoa(*v.UID))
}
if *v.GID != emptyVolumeDefaultGID {
s = append(s, ",gid=")
s = append(s, strconv.Itoa(*v.GID))
}
}
return strings.Join(s, "")
}

// VolumeFromString takes a command line volume parameter and returns a volume
//
// Example volume parameters:
// database,kind=host,source=/tmp,readOnly=true
func VolumeFromString(vp string) (*Volume, error) {
vol := Volume{
Mode: "0755",
UID: 0,
GID: 0,
}
var vol Volume

vp = "name=" + vp
vpQuery, err := common.MakeQueryString(vp)
Expand All @@ -138,6 +164,7 @@ func VolumeFromString(vp string) (*Volume, error) {
return nil, err
}
for key, val := range v {
val := val
Copy link
Contributor

@steveej steveej Nov 26, 2015

Choose a reason for hiding this comment

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

even if it works this looks weird

Copy link
Contributor Author

@jonboulle jonboulle Nov 26, 2015

Choose a reason for hiding this comment

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

it's idiomatic actually :/
https://golang.org/doc/effective_go.html
search for "req := req"

if len(val) > 1 {
return nil, fmt.Errorf("label %s with multiple values %q", key, val)
}
Expand All @@ -160,32 +187,50 @@ func VolumeFromString(vp string) (*Volume, error) {
}
vol.ReadOnly = &ro
case "mode":
vol.Mode = val[0]
vol.Mode = &val[0]
case "uid":
u, err := strconv.Atoi(val[0])
if err != nil {
return nil, err
}
vol.UID = u
vol.UID = &u
case "gid":
g, err := strconv.Atoi(val[0])
if err != nil {
return nil, err
}
vol.GID = g
vol.GID = &g
default:
return nil, fmt.Errorf("unknown volume parameter %q", key)
}
}

maybeSetDefaults(&vol)

err = vol.assertValid()
if err != nil {
return nil, err
}
if vol.Kind != "empty" {
vol.Mode = ""
vol.UID = -1
vol.GID = -1
}

return &vol, nil
}

// maybeSetDefaults sets the correct default values for certain fields on a
// Volume if they are not already been set. These fields are not
// pre-populated on all Volumes as the Volume type is polymorphic.
func maybeSetDefaults(vol *Volume) {
if vol.Kind == "empty" {
if vol.Mode == nil {
m := emptyVolumeDefaultMode
vol.Mode = &m
}
if vol.UID == nil {
u := emptyVolumeDefaultUID
vol.UID = &u
}
if vol.GID == nil {
g := emptyVolumeDefaultGID
vol.GID = &g
}
}
}
95 changes: 64 additions & 31 deletions schema/types/volume_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,19 @@ import (
"testing"
)

func TestVolumeFromString(t *testing.T) {
trueVar := true
falseVar := false
func bp(b bool) *bool {
return &b
}

func sp(s string) *string {
Copy link

@crawford crawford Nov 26, 2015

Choose a reason for hiding this comment

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

lol. I hate this language.

return &s
}

func ip(i int) *int {
return &i
}

func TestVolumeToFromString(t *testing.T) {
tests := []struct {
s string
v Volume
Expand All @@ -33,9 +43,9 @@ func TestVolumeFromString(t *testing.T) {
Kind: "host",
Source: "/tmp",
ReadOnly: nil,
Mode: "",
UID: -1,
GID: -1,
Mode: nil,
UID: nil,
GID: nil,
},
},
{
Expand All @@ -44,10 +54,10 @@ func TestVolumeFromString(t *testing.T) {
Name: "foobar",
Kind: "host",
Source: "/tmp",
ReadOnly: &falseVar,
Mode: "",
UID: -1,
GID: -1,
ReadOnly: bp(false),
Mode: nil,
UID: nil,
GID: nil,
},
},
{
Expand All @@ -56,10 +66,10 @@ func TestVolumeFromString(t *testing.T) {
Name: "foobar",
Kind: "host",
Source: "/tmp",
ReadOnly: &trueVar,
Mode: "",
UID: -1,
GID: -1,
ReadOnly: bp(true),
Mode: nil,
UID: nil,
GID: nil,
},
},
{
Expand All @@ -68,31 +78,31 @@ func TestVolumeFromString(t *testing.T) {
Name: "foobar",
Kind: "empty",
ReadOnly: nil,
Mode: "0755",
UID: 0,
GID: 0,
Mode: sp(emptyVolumeDefaultMode),
UID: ip(emptyVolumeDefaultUID),
GID: ip(emptyVolumeDefaultGID),
},
},
{
"foobar,kind=empty,readOnly=true",
Volume{
Name: "foobar",
Kind: "empty",
ReadOnly: &trueVar,
Mode: "0755",
UID: 0,
GID: 0,
ReadOnly: bp(true),
Mode: sp(emptyVolumeDefaultMode),
UID: ip(emptyVolumeDefaultUID),
GID: ip(emptyVolumeDefaultGID),
},
},
{
"foobar,kind=empty,readOnly=true,mode=0777",
Volume{
Name: "foobar",
Kind: "empty",
ReadOnly: &trueVar,
Mode: "0777",
UID: 0,
GID: 0,
ReadOnly: bp(true),
Mode: sp("0777"),
UID: ip(emptyVolumeDefaultUID),
GID: ip(emptyVolumeDefaultGID),
},
},
{
Expand All @@ -101,9 +111,9 @@ func TestVolumeFromString(t *testing.T) {
Name: "foobar",
Kind: "empty",
ReadOnly: nil,
Mode: "0777",
UID: 1000,
GID: 0,
Mode: sp("0777"),
UID: ip(1000),
GID: ip(emptyVolumeDefaultGID),
},
},
{
Expand All @@ -112,9 +122,9 @@ func TestVolumeFromString(t *testing.T) {
Name: "foobar",
Kind: "empty",
ReadOnly: nil,
Mode: "0777",
UID: 1000,
GID: 1000,
Mode: sp("0777"),
UID: ip(1000),
GID: ip(1000),
},
},
}
Expand All @@ -126,13 +136,22 @@ func TestVolumeFromString(t *testing.T) {
if !reflect.DeepEqual(*v, tt.v) {
t.Errorf("#%d: v=%v, want %v", i, *v, tt.v)
}
// volume serialization should be reversible
o := v.String()
if o != tt.s {
t.Errorf("#%d: v.String()=%s, want %s", i, o, tt.s)
}
}
}

func TestVolumeFromStringBad(t *testing.T) {
tests := []string{
"#foobar,kind=host,source=/tmp",
"foobar,kind=host,source=/tmp,readOnly=true,asdf=asdf",
"foobar,kind=host,source=tmp",
"foobar,kind=host,uid=3",
"foobar,kind=host,mode=0755",
"foobar,kind=host,mode=0600,readOnly=true,gid=0",
"foobar,kind=empty,source=/tmp",
}
for i, in := range tests {
Expand All @@ -145,3 +164,17 @@ func TestVolumeFromStringBad(t *testing.T) {
}
}
}

func BenchmarkVolumeToString(b *testing.B) {
vol := Volume{
Name: "foobar",
Kind: "empty",
ReadOnly: bp(true),
Mode: sp("0777"),
UID: ip(3),
GID: ip(4),
}
for i := 0; i < b.N; i++ {
_ = vol.String()
}
}