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

Improve error handling, fix #416 #462

Merged
merged 2 commits into from
Mar 15, 2017
Merged

Improve error handling, fix #416 #462

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5cb598f
return errors instead of logrus.Fatal calls
concaf Feb 20, 2017
e9544ca
add github.com/pkg/errors to glide and vendor
concaf Mar 1, 2017
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
26 changes: 14 additions & 12 deletions glide.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions glide.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import:
- package: github.com/ghodss/yaml
- package: github.com/spf13/cobra
- package: github.com/spf13/viper
- package: github.com/pkg/errors

- package: github.com/docker/libcompose
version: 8e4221d0435d29e6239adf9ccb4de1f0c0ab0935
Expand Down
26 changes: 21 additions & 5 deletions pkg/app/app.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,16 +211,26 @@ func Convert(opt kobject.ConvertOptions) {
komposeObject := kobject.KomposeObject{
ServiceConfigs: make(map[string]kobject.ServiceConfig),
}
komposeObject = l.LoadFile(opt.InputFiles)
komposeObject, err = l.LoadFile(opt.InputFiles)
if err != nil {
log.Fatalf(err.Error())
}

// Get a transformer that maps komposeObject to provider's primitives
t := getTransformer(opt)

// Do the transformation
objects := t.Transform(komposeObject, opt)
objects, err := t.Transform(komposeObject, opt)

if err != nil {
log.Fatalf(err.Error())
}

// Print output
kubernetes.PrintList(objects, opt)
err = kubernetes.PrintList(objects, opt)
if err != nil {
log.Fatalf(err.Error())
}
}

// Up brings up deployment, svc.
Expand All @@ -237,7 +247,10 @@ func Up(opt kobject.ConvertOptions) {
komposeObject := kobject.KomposeObject{
ServiceConfigs: make(map[string]kobject.ServiceConfig),
}
komposeObject = l.LoadFile(opt.InputFiles)
komposeObject, err = l.LoadFile(opt.InputFiles)
if err != nil {
log.Fatalf(err.Error())
}

// Get the transformer
t := getTransformer(opt)
Expand All @@ -263,7 +276,10 @@ func Down(opt kobject.ConvertOptions) {
komposeObject := kobject.KomposeObject{
ServiceConfigs: make(map[string]kobject.ServiceConfig),
}
komposeObject = l.LoadFile(opt.InputFiles)
komposeObject, err = l.LoadFile(opt.InputFiles)
if err != nil {
log.Fatalf(err.Error())
}

// Get the transformer
t := getTransformer(opt)
Expand Down
37 changes: 19 additions & 18 deletions pkg/loader/bundle/bundle.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import (
log "github.com/Sirupsen/logrus"
"github.com/fatih/structs"
"github.com/kubernetes-incubator/kompose/pkg/kobject"
"github.com/pkg/errors"
)

// Bundle is docker bundle file loader, implements Loader interface
Expand Down Expand Up @@ -106,16 +107,16 @@ func checkUnsupportedKey(bundleStruct *Bundlefile) []string {
}

// load image from dab file
func loadImage(service Service) (string, string) {
func loadImage(service Service) (string, error) {
character := "@"
if strings.Contains(service.Image, character) {
return service.Image[0:strings.Index(service.Image, character)], ""
return service.Image[0:strings.Index(service.Image, character)], nil
}
return "", "Invalid image format"
return "", errors.New("Invalid image format")
}

// load environment variables from dab file
func loadEnvVars(service Service) ([]kobject.EnvVar, string) {
func loadEnvVars(service Service) ([]kobject.EnvVar, error) {
envs := []kobject.EnvVar{}
for _, env := range service.Env {
character := "="
Expand Down Expand Up @@ -144,15 +145,15 @@ func loadEnvVars(service Service) ([]kobject.EnvVar, string) {
Value: value,
})
} else {
return envs, "Invalid container env " + env
return envs, errors.New("Invalid container env")
}
}
}
return envs, ""
return envs, nil
}

// load ports from dab file
func loadPorts(service Service) ([]kobject.Ports, string) {
func loadPorts(service Service) ([]kobject.Ports, error) {
ports := []kobject.Ports{}
for _, port := range service.Ports {
var p api.Protocol
Expand All @@ -170,24 +171,24 @@ func loadPorts(service Service) ([]kobject.Ports, string) {
Protocol: p,
})
}
return ports, ""
return ports, nil
}

// LoadFile loads dab file into KomposeObject
func (b *Bundle) LoadFile(files []string) kobject.KomposeObject {
func (b *Bundle) LoadFile(files []string) (kobject.KomposeObject, error) {
komposeObject := kobject.KomposeObject{
ServiceConfigs: make(map[string]kobject.ServiceConfig),
LoadedFrom: "bundle",
}
file := files[0]
buf, err := ioutil.ReadFile(file)
if err != nil {
log.Fatalf("Failed to read bundles file: %s ", err)
return kobject.KomposeObject{}, errors.Wrap(err, "ioutil.ReadFile failed, Failed to read bundles file")
Copy link
Member

Choose a reason for hiding this comment

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

Just curious: Is it good practice to send the method/function name in error?

Copy link
Member

Choose a reason for hiding this comment

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

Also follow up question here you are using the error message as ioutil.ReadFile failed which is kinda absolute name but below at line#191 you have just used the name which is loadFile, why is that so?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@surajssd about the good practice part, it's a very subjective opinion, but we can use it to track down the error when the error messages will get printed in app.go

and about the the error message part, I added the name of the failing functions wrt the function they were failing in, so it gets easier to track down the error.

}
reader := strings.NewReader(string(buf))
bundle, err := loadFile(reader)
if err != nil {
log.Fatalf("Failed to parse bundles file: %s", err)
return kobject.KomposeObject{}, errors.Wrap(err, "loadFile failed, Failed to parse bundles file")
}

noSupKeys := checkUnsupportedKey(bundle)
Expand All @@ -204,20 +205,20 @@ func (b *Bundle) LoadFile(files []string) kobject.KomposeObject {
serviceConfig.Annotations = service.Labels

image, err := loadImage(service)
if err != "" {
log.Fatalf("Failed to load image from bundles file: " + err)
if err != nil {
return kobject.KomposeObject{}, errors.Wrap(err, "loadImage failed, Failed to load image from bundles file")
}
serviceConfig.Image = image

envs, err := loadEnvVars(service)
if err != "" {
log.Fatalf("Failed to load envvar from bundles file: " + err)
if err != nil {
return kobject.KomposeObject{}, errors.Wrap(err, "loadEnvVars failed, Failed to load envvar from bundles file")
}
serviceConfig.Environment = envs

ports, err := loadPorts(service)
if err != "" {
log.Fatalf("Failed to load ports from bundles file: " + err)
if err != nil {
return kobject.KomposeObject{}, errors.Wrap(err, "loadPorts failed, Failed to load ports from bundles file")
}
serviceConfig.Port = ports

Expand All @@ -228,7 +229,7 @@ func (b *Bundle) LoadFile(files []string) kobject.KomposeObject {
komposeObject.ServiceConfigs[name] = serviceConfig
}

return komposeObject
return komposeObject, nil
}

// LoadFile loads a bundlefile from a path to the file
Expand Down
29 changes: 17 additions & 12 deletions pkg/loader/compose/compose.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import (
"github.com/docker/libcompose/project"
"github.com/fatih/structs"
"github.com/kubernetes-incubator/kompose/pkg/kobject"
"github.com/pkg/errors"
)

// Compose is docker compose file loader, implements Loader interface
Expand Down Expand Up @@ -275,7 +276,7 @@ func loadPorts(composePorts []string) ([]kobject.Ports, error) {
}

// LoadFile loads compose file into KomposeObject
func (c *Compose) LoadFile(files []string) kobject.KomposeObject {
func (c *Compose) LoadFile(files []string) (kobject.KomposeObject, error) {
komposeObject := kobject.KomposeObject{
ServiceConfigs: make(map[string]kobject.ServiceConfig),
LoadedFrom: "compose",
Expand All @@ -290,7 +291,7 @@ func (c *Compose) LoadFile(files []string) kobject.KomposeObject {
if context.EnvironmentLookup == nil {
cwd, err := os.Getwd()
if err != nil {
return kobject.KomposeObject{}
return kobject.KomposeObject{}, nil
}
context.EnvironmentLookup = &lookup.ComposableEnvLookup{
Lookups: []config.EnvironmentLookup{
Expand All @@ -306,7 +307,7 @@ func (c *Compose) LoadFile(files []string) kobject.KomposeObject {
composeObject := project.NewProject(context, nil, nil)
err := composeObject.Parse()
if err != nil {
log.Fatalf("Failed to load compose file: %v", err)
return kobject.KomposeObject{}, errors.Wrap(err, "composeObject.Parse() failed, Failed to load compose file")
}

noSupKeys := checkUnsupportedKey(composeObject)
Expand All @@ -329,7 +330,7 @@ func (c *Compose) LoadFile(files []string) kobject.KomposeObject {
// load ports
ports, err := loadPorts(composeServiceConfig.Ports)
if err != nil {
log.Fatalf("%q failed to load ports from compose file: %v", name, err)
return kobject.KomposeObject{}, errors.Wrap(err, "loadPorts failed. "+name+" failed to load ports from compose file")
}
serviceConfig.Port = ports

Expand All @@ -347,7 +348,12 @@ func (c *Compose) LoadFile(files []string) kobject.KomposeObject {
for key, value := range composeServiceConfig.Labels {
switch key {
case "kompose.service.type":
serviceConfig.ServiceType = handleServiceType(value)
serviceType, err := handleServiceType(value)
if err != nil {
return kobject.KomposeObject{}, errors.Wrap(err, "handleServiceType failed")
}

serviceConfig.ServiceType = serviceType
case "kompose.service.expose":
serviceConfig.ExposeService = strings.ToLower(value)
}
Expand All @@ -373,20 +379,19 @@ func (c *Compose) LoadFile(files []string) kobject.KomposeObject {
komposeObject.ServiceConfigs[name] = serviceConfig
}

return komposeObject
return komposeObject, nil
}

func handleServiceType(ServiceType string) string {
func handleServiceType(ServiceType string) (string, error) {
switch strings.ToLower(ServiceType) {
case "", "clusterip":
return string(api.ServiceTypeClusterIP)
return string(api.ServiceTypeClusterIP), nil
case "nodeport":
return string(api.ServiceTypeNodePort)
return string(api.ServiceTypeNodePort), nil
case "loadbalancer":
return string(api.ServiceTypeLoadBalancer)
return string(api.ServiceTypeLoadBalancer), nil
default:
log.Fatalf("Unknown value '%s', supported values are 'NodePort, ClusterIP or LoadBalancer'", ServiceType)
return ""
return "", errors.New("Unknown value " + ServiceType + " , supported values are 'NodePort, ClusterIP or LoadBalancer'")
}
}

Expand Down
6 changes: 5 additions & 1 deletion pkg/loader/compose/compose_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import (
"github.com/docker/libcompose/config"
"github.com/docker/libcompose/project"
"github.com/docker/libcompose/yaml"
"github.com/pkg/errors"
)

// Test if service types are parsed properly on user input
Expand All @@ -47,7 +48,10 @@ func TestHandleServiceType(t *testing.T) {
}

for _, tt := range tests {
result := handleServiceType(tt.labelValue)
result, err := handleServiceType(tt.labelValue)
if err != nil {
t.Error(errors.Wrap(err, "handleServiceType failed"))
}
if result != tt.serviceType {
t.Errorf("Expected %q, got %q", tt.serviceType, result)
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/loader/loader.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ import (

// Loader interface defines loader that loads files and converts it to kobject representation
type Loader interface {
LoadFile(files []string) kobject.KomposeObject
LoadFile(files []string) (kobject.KomposeObject, error)
///Name() string
}

Expand Down
Loading