Fix code after review

Co-authored-by: Chris <github.account@chrigel.net>

go.mod

@@ -9,7 +9,7 @@ require (
 	github.com/go-logr/logr v1.2.3
 	github.com/go-logr/zapr v1.2.3
 	github.com/jmoiron/sqlx v1.3.5
-	github.com/urfave/cli/v2 v2.16.3
+	github.com/urfave/cli/v2 v2.17.1
 	github.com/vshn/cloudscale-metrics-collector v0.3.3
 	github.com/vshn/provider-exoscale v0.1.0
 	go.uber.org/zap v1.23.0

go.sum

@@ -452,8 +452,8 @@ github.com/ugorji/go v1.1.7/go.mod h1:kZn38zHttfInRq0xu/PH0az30d+z6vm202qpg1oXVM
 github.com/ugorji/go v1.2.6/go.mod h1:anCg0y61KIhDlPZmnH+so+RQbysYVyDko0IMgJv0Nn0=
 github.com/ugorji/go/codec v1.1.7/go.mod h1:Ax+UKWsSmolVDwsd+7N3ZtXu+yMGCf907BLYF3GoBXY=
 github.com/ugorji/go/codec v1.2.6/go.mod h1:V6TCNZ4PHqoHGFZuSG1W8nrCzzdgA2DozYxWFFpvxTw=
-github.com/urfave/cli/v2 v2.16.3 h1:gHoFIwpPjoyIMbJp/VFd+/vuD0dAgFK4B6DpEMFJfQk=
-github.com/urfave/cli/v2 v2.16.3/go.mod h1:1CNUng3PtjQMtRzJO4FMXBQvkGtuYRxxiR9xMa7jMwI=
+github.com/urfave/cli/v2 v2.17.1 h1:UzjDEw2dJQUE3iRaiNQ1VrVFbyAtKGH3VdkMoHA58V0=
+github.com/urfave/cli/v2 v2.17.1/go.mod h1:1CNUng3PtjQMtRzJO4FMXBQvkGtuYRxxiR9xMa7jMwI=
 github.com/valyala/bytebufferpool v1.0.0/go.mod h1:6bBcMArwyJ5K/AmCkWv1jt77kVWyCJ6HpOuEn7z0Csc=
 github.com/valyala/fasttemplate v1.2.1/go.mod h1:KHLXt3tVN2HBp8eijSv/kGJopbvo7S+qRAEEKiv+SiQ=
 github.com/vshn/cloudscale-metrics-collector v0.3.3 h1:81y66v4WV1dPYYf+gpPwyxHa2bDb+igSXXUIZF/1vrg=

pkg/database/database.go

@@ -7,6 +7,7 @@ import (
 	"github.com/appuio/appuio-cloud-reporting/pkg/db"
 	pipeline "github.com/ccremer/go-command-pipeline"
 	"github.com/jmoiron/sqlx"
+	"github.com/urfave/cli/v2"
 	"github.com/vshn/cloudscale-metrics-collector/pkg/categoriesmodel"
 	"github.com/vshn/cloudscale-metrics-collector/pkg/datetimesmodel"
 	"github.com/vshn/cloudscale-metrics-collector/pkg/discountsmodel"
@@ -76,12 +77,12 @@ type Database struct {
 }
 
 // ValidateDatabaseURL validates environment variable dbUrlEnvVariable which holds the database URL
-func ValidateDatabaseURL(databaseURL string) (database *Database, err error) {
-	_, err = url.ParseRequestURI(databaseURL)
+func ValidateDatabaseURL(_ *cli.Context, databaseURL string) error {
+	_, err := url.ParseRequestURI(databaseURL)
 	if err != nil {
-		return nil, fmt.Errorf("database url is not valid")
+		return fmt.Errorf("database url is not valid")
 	}
-	return &Database{Url: databaseURL}, nil
+	return nil
 }
 
 // OpenConnection opens a connection to the postgres database
@@ -100,7 +101,7 @@ func (d *Database) CloseConnection() error {
 	if err != nil {
 		return fmt.Errorf("cannot close database connection: %w", err)
 	}
-	return err
+	return nil
 }
 
 // EnsureBucketUsage saves the aggregated buckets usage by namespace to the postgresql database
@@ -121,13 +122,18 @@ func (d *Database) EnsureBucketUsage(ctx context.Context, namespace string, aggr
 		p.NewStep("Save facts", d.saveFacts),
 		p.NewStep("Commit transaction", d.commitTransaction),
 	)
-	return p.RunWithContext(ctx)
+	err := p.RunWithContext(ctx)
+	if err != nil {
+		d.transaction.Rollback()
+		return fmt.Errorf("could not save to database: %w", err)
+	}
+	return nil
 }
 
 func (d *Database) beginTransaction(ctx context.Context) error {
 	tx, err := d.connection.BeginTxx(ctx, &sql.TxOptions{})
 	if err != nil {
-		return fmt.Errorf("cannot create database transaction for namespace %s: %w", d.namespace, err)
+		return fmt.Errorf("cannot create database transaction for namespace %s: %w", *d.namespace, err)
 	}
 	d.transaction = tx
 	return nil
@@ -190,40 +196,40 @@ func (d *Database) saveFacts(ctx context.Context) error {
 	storageFact := factsmodel.New(d.dateTime, d.query, d.tenant, d.category, d.product, d.discount, *d.quantity)
 	_, err := factsmodel.Ensure(ctx, d.transaction, storageFact)
 	if err != nil {
-		return fmt.Errorf("cannot save fact for namespace %s: %w", d.namespace, err)
+		return fmt.Errorf("cannot save fact for namespace %s: %w", *d.namespace, err)
 	}
 	return nil
 }
 
 func (d *Database) commitTransaction(_ context.Context) error {
 	err := d.transaction.Commit()
 	if err != nil {
-		return fmt.Errorf("cannot commit transaction for buckets in namespace %s: %w", d.namespace, err)
+		return fmt.Errorf("cannot commit transaction for buckets in namespace %s: %w", *d.namespace, err)
 	}
 	return nil
 }
 
 // EnsureInitConfiguration ensures the minimum exoscale object storage configuration data is present in the database
 // before saving buckets usage
 func (d *Database) EnsureInitConfiguration(ctx context.Context) error {
-	tx, err := d.connection.BeginTxx(ctx, &sql.TxOptions{})
+	transaction, err := d.connection.BeginTxx(ctx, &sql.TxOptions{})
 	if err != nil {
 		return fmt.Errorf("cannot begin transaction for initial database configuration: %w", err)
 	}
-	defer tx.Rollback()
-	_, err = productsmodel.Ensure(ctx, tx, &product)
+	defer transaction.Rollback()
+	_, err = productsmodel.Ensure(ctx, transaction, &product)
 	if err != nil {
 		return fmt.Errorf("cannot ensure exoscale product model in the database: %w", err)
 	}
-	_, err = discountsmodel.Ensure(ctx, tx, &discount)
+	_, err = discountsmodel.Ensure(ctx, transaction, &discount)
 	if err != nil {
 		return fmt.Errorf("cannot ensure exoscale discount model in the database: %w", err)
 	}
-	_, err = queriesmodel.Ensure(ctx, tx, &query)
+	_, err = queriesmodel.Ensure(ctx, transaction, &query)
 	if err != nil {
 		return fmt.Errorf("cannot ensure exoscale query model in the database: %w", err)
 	}
-	err = tx.Commit()
+	err = transaction.Commit()
 	if err != nil {
 		return fmt.Errorf("cannot commit transaction for initial database configuration: %w", err)
 	}

pkg/exoscale/exoscale.go

@@ -15,5 +15,5 @@ func InitClient(exoscaleAccessKey, exoscaleSecret string) (*egoscale.Client, err
 	if err != nil {
 		return nil, fmt.Errorf("cannot create Exoscale client: %w", err)
 	}
-	return client, err
+	return client, nil
 }

pkg/sos/objectstorage.go

@@ -35,11 +35,11 @@ type BucketDetail struct {
 	Organization, BucketName, Namespace string
 }
 
-func NewObjectStorage(exoscaleClient *egoscale.Client, k8sClient *k8s.Client, database *db.Database) ObjectStorage {
+func NewObjectStorage(exoscaleClient *egoscale.Client, k8sClient *k8s.Client, databaseURL string) ObjectStorage {
 	return ObjectStorage{
 		exoscaleClient: exoscaleClient,
 		k8sClient:      k8sClient,
-		database:       database,
+		database:       &db.Database{Url: databaseURL},
 	}
 }
 

sos_command.go

@@ -19,60 +19,59 @@ const (
 )
 
 type objectStorageCommand struct {
-	clusterURL       string
-	clusterToken     string
-	databaseURL      string
-	exoscaleKey      string
-	exoscaleSecret   string
-	databaseInstance *db.Database
+	clusterURL     string
+	clusterToken   string
+	databaseURL    string
+	exoscaleKey    string
+	exoscaleSecret string
 }
 
 func NewCommand() *cli.Command {
 	command := &objectStorageCommand{}
 	return &cli.Command{
 		Name:   objectStorageName,
 		Usage:  "Get metrics from object storage service",
-		Before: command.validate,
 		Action: command.execute,
 		Flags: []cli.Flag{
 			&cli.StringFlag{
 				Name:        "k8s-server-url",
 				Aliases:     []string{"u"},
 				EnvVars:     []string{k8sServerUrlEnvVariable},
 				Required:    true,
-				Usage:       "A kubernetes server url from where to get the data from",
+				Usage:       "A Kubernetes server URL from where to get the data from",
 				Destination: &command.clusterURL,
 			},
 			&cli.StringFlag{
 				Name:        "k8s-server-token",
 				Aliases:     []string{"t"},
 				EnvVars:     []string{k8sTokenEnvVariable},
 				Required:    true,
-				Usage:       "A kubernetes server token which has access to buckets",
+				Usage:       "A Kubernetes server token which can view buckets.exoscale.crossplane.io resources",
 				Destination: &command.clusterToken,
 			},
 			&cli.StringFlag{
 				Name:        "database-url",
 				Aliases:     []string{"d"},
 				EnvVars:     []string{dbUrlEnvVariable},
+				Action:      db.ValidateDatabaseURL,
 				Required:    true,
-				Usage:       "A postgres database url where to save relevant metrics",
+				Usage:       "A PostgreSQL database URL where to save relevant metrics",
 				Destination: &command.databaseURL,
 			},
 			&cli.StringFlag{
 				Name:        "exoscale-access-key",
 				Aliases:     []string{"k"},
 				EnvVars:     []string{keyEnvVariable},
 				Required:    true,
-				Usage:       "A key which has full access to an exoscale organization S3 storage",
+				Usage:       "A key which has unrestricted SOS service access in an Exoscale organization",
 				Destination: &command.exoscaleKey,
 			},
 			&cli.StringFlag{
 				Name:        "exoscale-secret",
 				Aliases:     []string{"s"},
 				EnvVars:     []string{secretEnvVariable},
 				Required:    true,
-				Usage:       "A secret which has full access to an exoscale organization S3 storage",
+				Usage:       "The secret which has unrestricted SOS service access in an Exoscale organization",
 				Destination: &command.exoscaleSecret,
 			},
 		},
@@ -81,6 +80,7 @@ func NewCommand() *cli.Command {
 
 func (c *objectStorageCommand) execute(ctx *cli.Context) error {
 	log := AppLogger(ctx).WithName(objectStorageName)
+	ctrl.SetLogger(log)
 
 	log.Info("Creating Exoscale client")
 	exoscaleClient, err := exoscale.InitClient(c.exoscaleKey, c.exoscaleSecret)
@@ -94,21 +94,6 @@ func (c *objectStorageCommand) execute(ctx *cli.Context) error {
 		return err
 	}
 
-	o := sos.NewObjectStorage(exoscaleClient, k8sClient, c.databaseInstance)
+	o := sos.NewObjectStorage(exoscaleClient, k8sClient, c.databaseURL)
 	return o.Execute(ctx.Context)
 }
-
-func (c *objectStorageCommand) validate(ctx *cli.Context) error {
-	_ = LogMetadata(ctx)
-	log := AppLogger(ctx).WithName(objectStorageName)
-	ctrl.SetLogger(log)
-
-	log.Info("Validate database URL")
-	databaseInstance, err := db.ValidateDatabaseURL(c.databaseURL)
-	if err != nil {
-		return err
-	}
-	c.databaseInstance = databaseInstance
-
-	return nil
-}