17187: implement review feedback:

* uuids are now specified at the end of the option list
* invalid uuids will cause the command to error out
* output directory is now optional

Arvados-DCO-1.1-Signed-off-by: Ward Vandewege <ward@curii.com>

diff --git a/lib/costanalyzer/costanalyzer.go b/lib/costanalyzer/costanalyzer.go
index bca23b1535c6440e49bbf8029b28e7da3beabc76..d81ade607c6ebc1b1426220077b60c9fa3cad77d 100644 (file)
--- a/lib/costanalyzer/costanalyzer.go
+++ b/lib/costanalyzer/costanalyzer.go
@@ -56,12 +56,12 @@ func parseFlags(prog string, args []string, loader *config.Loader, logger *logru
        flags.Usage = func() {
                fmt.Fprintf(flags.Output(), `
 Usage:
-  %s [options ...]
+  %s [options ...] <uuid> ...
 
        This program analyzes the cost of Arvados container requests. For each uuid
        supplied, it creates a CSV report that lists all the containers used to
        fulfill the container request, together with the machine type and cost of
-       each container.
+       each container. At least one uuid must be specified.
 
        When supplied with the uuid of a container request, it will calculate the
        cost of that container request and all its children.
@@ -97,13 +97,15 @@ Usage:
        This program prints the total dollar amount from the aggregate cost
        accounting across all provided uuids on stdout.
 
+       When the '-output' option is specified, a set of CSV files with cost details
+       will be written to the provided directory.
+
 Options:
 `, prog)
                flags.PrintDefaults()
        }
        loglevel := flags.String("log-level", "info", "logging `level` (debug, info, ...)")
-       flags.StringVar(&resultsDir, "output", "", "output `directory` for the CSV reports (required)")
-       flags.Var(&uuids, "uuid", "object uuid. May be specified more than once. Also accepts a comma separated list of uuids (required)")
+       flags.StringVar(&resultsDir, "output", "", "output `directory` for the CSV reports")
        flags.BoolVar(&cache, "cache", true, "create and use a local disk cache of Arvados objects")
        err = flags.Parse(args)
        if err == flag.ErrHelp {
@@ -114,6 +116,7 @@ Options:
                exitCode = 2
                return
        }
+       uuids = flags.Args()
 
        if len(uuids) < 1 {
                flags.Usage()
@@ -122,13 +125,6 @@ Options:
                return
        }
 
-       if resultsDir == "" {
-               flags.Usage()
-               err = fmt.Errorf("Error: output directory must be specified")
-               exitCode = 2
-               return
-       }
-
        lvl, err := logrus.ParseLevel(*loglevel)
        if err != nil {
                exitCode = 2
@@ -390,7 +386,10 @@ func generateCrCsv(logger *logrus.Logger, uuid string, arv *arvadosclient.Arvado
                if !ok {
                        return nil, fmt.Errorf("error: collection %s does not have a 'container_request' property", uuid)
                }
-               crUUID = value.(string)
+               crUUID, ok = value.(string)
+               if !ok {
+                       return nil, fmt.Errorf("error: collection %s does not have a 'container_request' property of the string type", uuid)
+               }
        }
 
        // This is a container request, find the container
@@ -451,13 +450,15 @@ func generateCrCsv(logger *logrus.Logger, uuid string, arv *arvadosclient.Arvado
 
        csv += "TOTAL,,,,,,,,," + strconv.FormatFloat(totalCost, 'f', 8, 64) + "\n"
 
-       // Write the resulting CSV file
-       fName := resultsDir + "/" + uuid + ".csv"
-       err = ioutil.WriteFile(fName, []byte(csv), 0644)
-       if err != nil {
-               return nil, fmt.Errorf("error writing file with path %s: %s", fName, err.Error())
+       if resultsDir != "" {
+               // Write the resulting CSV file
+               fName := resultsDir + "/" + uuid + ".csv"
+               err = ioutil.WriteFile(fName, []byte(csv), 0644)
+               if err != nil {
+                       return nil, fmt.Errorf("error writing file with path %s: %s", fName, err.Error())
+               }
+               logger.Infof("\nUUID report in %s\n\n", fName)
        }
-       logger.Infof("\nUUID report in %s\n\n", fName)
 
        return
 }
@@ -467,10 +468,12 @@ func costanalyzer(prog string, args []string, loader *config.Loader, logger *log
        if exitcode != 0 {
                return
        }
-       err = ensureDirectory(logger, resultsDir)
-       if err != nil {
-               exitcode = 3
-               return
+       if resultsDir != "" {
+               err = ensureDirectory(logger, resultsDir)
+               if err != nil {
+                       exitcode = 3
+                       return
+               }
        }
 
        // Arvados Client setup
@@ -519,6 +522,10 @@ func costanalyzer(prog string, args []string, loader *config.Loader, logger *log
                        // "Home" project is not supported by this program. Skip this uuid, but
                        // keep going.
                        logger.Errorf("Cost analysis is not supported for the 'Home' project: %s", uuid)
+               } else {
+                       logger.Errorf("This argument does not look like a uuid: %s\n", uuid)
+                       exitcode = 3
+                       return
                }
        }
 
@@ -542,15 +549,18 @@ func costanalyzer(prog string, args []string, loader *config.Loader, logger *log
 
        csv += "TOTAL," + strconv.FormatFloat(total, 'f', 8, 64) + "\n"
 
-       // Write the resulting CSV file
-       aFile := resultsDir + "/" + time.Now().Format("2006-01-02-15-04-05") + "-aggregate-costaccounting.csv"
-       err = ioutil.WriteFile(aFile, []byte(csv), 0644)
-       if err != nil {
-               err = fmt.Errorf("Error writing file with path %s: %s", aFile, err.Error())
-               exitcode = 1
-               return
+       if resultsDir != "" {
+               // Write the resulting CSV file
+               aFile := resultsDir + "/" + time.Now().Format("2006-01-02-15-04-05") + "-aggregate-costaccounting.csv"
+               err = ioutil.WriteFile(aFile, []byte(csv), 0644)
+               if err != nil {
+                       err = fmt.Errorf("Error writing file with path %s: %s", aFile, err.Error())
+                       exitcode = 1
+                       return
+               }
+               logger.Infof("Aggregate cost accounting for all supplied uuids in %s\n", aFile)
        }
-       logger.Infof("Aggregate cost accounting for all supplied uuids in %s\n", aFile)
+
        // Output the total dollar amount on stdout
        fmt.Fprintf(stdout, "%s\n", strconv.FormatFloat(total, 'f', 8, 64))
 

diff --git a/lib/costanalyzer/costanalyzer_test.go b/lib/costanalyzer/costanalyzer_test.go
index 48e7733e1878ba9ff0524b62edd7b8421f33137f..3488f0d8dfd62522a7bfd4774f0ac8d7b903a88b 100644 (file)
--- a/lib/costanalyzer/costanalyzer_test.go
+++ b/lib/costanalyzer/costanalyzer_test.go
@@ -161,7 +161,7 @@ func (*Suite) TestUsage(c *check.C) {
 func (*Suite) TestContainerRequestUUID(c *check.C) {
        var stdout, stderr bytes.Buffer
        // Run costanalyzer with 1 container request uuid
-       exitcode := Command.RunCommand("costanalyzer.test", []string{"-uuid", arvadostest.CompletedContainerRequestUUID, "-output", "results"}, &bytes.Buffer{}, &stdout, &stderr)
+       exitcode := Command.RunCommand("costanalyzer.test", []string{"-output", "results", arvadostest.CompletedContainerRequestUUID}, &bytes.Buffer{}, &stdout, &stderr)
        c.Check(exitcode, check.Equals, 0)
        c.Assert(stderr.String(), check.Matches, "(?ms).*supplied uuids in .*")
 
@@ -181,7 +181,7 @@ func (*Suite) TestCollectionUUID(c *check.C) {
        var stdout, stderr bytes.Buffer
 
        // Run costanalyzer with 1 collection uuid, without 'container_request' property
-       exitcode := Command.RunCommand("costanalyzer.test", []string{"-uuid", arvadostest.FooCollection, "-output", "results"}, &bytes.Buffer{}, &stdout, &stderr)
+       exitcode := Command.RunCommand("costanalyzer.test", []string{"-output", "results", arvadostest.FooCollection}, &bytes.Buffer{}, &stdout, &stderr)
        c.Check(exitcode, check.Equals, 2)
        c.Assert(stderr.String(), check.Matches, "(?ms).*does not have a 'container_request' property.*")
 
@@ -203,7 +203,7 @@ func (*Suite) TestCollectionUUID(c *check.C) {
        stderr.Truncate(0)
 
        // Run costanalyzer with 1 collection uuid
-       exitcode = Command.RunCommand("costanalyzer.test", []string{"-uuid", arvadostest.FooCollection, "-output", "results"}, &bytes.Buffer{}, &stdout, &stderr)
+       exitcode = Command.RunCommand("costanalyzer.test", []string{"-output", "results", arvadostest.FooCollection}, &bytes.Buffer{}, &stdout, &stderr)
        c.Check(exitcode, check.Equals, 0)
        c.Assert(stderr.String(), check.Matches, "(?ms).*supplied uuids in .*")
 
@@ -222,7 +222,7 @@ func (*Suite) TestCollectionUUID(c *check.C) {
 func (*Suite) TestDoubleContainerRequestUUID(c *check.C) {
        var stdout, stderr bytes.Buffer
        // Run costanalyzer with 2 container request uuids
-       exitcode := Command.RunCommand("costanalyzer.test", []string{"-uuid", arvadostest.CompletedContainerRequestUUID, "-uuid", arvadostest.CompletedContainerRequestUUID2, "-output", "results"}, &bytes.Buffer{}, &stdout, &stderr)
+       exitcode := Command.RunCommand("costanalyzer.test", []string{"-output", "results", arvadostest.CompletedContainerRequestUUID, arvadostest.CompletedContainerRequestUUID2}, &bytes.Buffer{}, &stdout, &stderr)
        c.Check(exitcode, check.Equals, 0)
        c.Assert(stderr.String(), check.Matches, "(?ms).*supplied uuids in .*")
 
@@ -262,7 +262,7 @@ func (*Suite) TestDoubleContainerRequestUUID(c *check.C) {
        c.Assert(err, check.IsNil)
 
        // Run costanalyzer with the project uuid
-       exitcode = Command.RunCommand("costanalyzer.test", []string{"-uuid", arvadostest.AProjectUUID, "-cache=false", "-log-level", "debug", "-output", "results"}, &bytes.Buffer{}, &stdout, &stderr)
+       exitcode = Command.RunCommand("costanalyzer.test", []string{"-cache=false", "-log-level", "debug", "-output", "results", arvadostest.AProjectUUID}, &bytes.Buffer{}, &stdout, &stderr)
        c.Check(exitcode, check.Equals, 0)
        c.Assert(stderr.String(), check.Matches, "(?ms).*supplied uuids in .*")
 
@@ -285,10 +285,10 @@ func (*Suite) TestDoubleContainerRequestUUID(c *check.C) {
 
 func (*Suite) TestMultipleContainerRequestUUIDWithReuse(c *check.C) {
        var stdout, stderr bytes.Buffer
-       // Run costanalyzer with 2 container request uuids, as one comma separated -uuid argument
-       exitcode := Command.RunCommand("costanalyzer.test", []string{"-uuid", arvadostest.CompletedDiagnosticsContainerRequest1UUID + "," + arvadostest.CompletedDiagnosticsContainerRequest2UUID, "-output", "results"}, &bytes.Buffer{}, &stdout, &stderr)
+       // Run costanalyzer with 2 container request uuids, without output directory specified
+       exitcode := Command.RunCommand("costanalyzer.test", []string{arvadostest.CompletedDiagnosticsContainerRequest1UUID, arvadostest.CompletedDiagnosticsContainerRequest2UUID}, &bytes.Buffer{}, &stdout, &stderr)
        c.Check(exitcode, check.Equals, 0)
-       c.Assert(stderr.String(), check.Matches, "(?ms).*supplied uuids in .*")
+       c.Assert(stderr.String(), check.Not(check.Matches), "(?ms).*supplied uuids in .*")
 
        // Check that the total amount was printed to stdout
        c.Check(stdout.String(), check.Matches, "0.01492030\n")
@@ -297,7 +297,7 @@ func (*Suite) TestMultipleContainerRequestUUIDWithReuse(c *check.C) {
        stderr.Truncate(0)
 
        // Run costanalyzer with 2 container request uuids
-       exitcode = Command.RunCommand("costanalyzer.test", []string{"-uuid", arvadostest.CompletedDiagnosticsContainerRequest1UUID, "-uuid", arvadostest.CompletedDiagnosticsContainerRequest2UUID, "-output", "results"}, &bytes.Buffer{}, &stdout, &stderr)
+       exitcode = Command.RunCommand("costanalyzer.test", []string{"-output", "results", arvadostest.CompletedDiagnosticsContainerRequest1UUID, arvadostest.CompletedDiagnosticsContainerRequest2UUID}, &bytes.Buffer{}, &stdout, &stderr)
        c.Check(exitcode, check.Equals, 0)
        c.Assert(stderr.String(), check.Matches, "(?ms).*supplied uuids in .*")