Add support for providing command line options. by supriyagarg · Pull Request #108 · Stackdriver/metadata-agent

supriyagarg · 2018-03-30T17:56:31Z

No description provided.

igorpeshansky · 2018-03-30T18:04:36Z

      ("version,V", "Print the agent version")
      ("verbose,v", boost::program_options::bool_switch(&verbose_logging_),
           "Enable verbose logging")
+      ("options,o",


Let's change options to option.

igorpeshansky · 2018-03-30T18:07:32Z

+      ("options,o",
+            boost::program_options::value<std::vector<std::string>>()
+                ->multitoken()->zero_tokens()->composing(),
+            "Command line options")


How about "Explicit configuration option, e.g., -o CredentialsFile=/tmp/token.json"?

igorpeshansky · 2018-03-30T18:08:18Z

+        std::cout << "Options:\n" << input_str;
+      }
+      std::istringstream input { input_str };
+      ParseConfiguration(input);


You can inline the std::istringstream construction into the ParseConfiguration call.

outdated line

igorpeshansky · 2018-03-30T18:11:47Z

                << std::endl;
      return -1;
    }
+    if (flags.count("options")) {


We should probably do this after the ParseConfigFile call, to allow explicit config option settings to override the configuration file.

Done. Also updated ParseConfiguration to actually pick up only the updated values.

igorpeshansky · 2018-03-30T18:13:38Z

+          flags["options"].as<std::vector<std::string>>();
+      for (const std::string& option: options) {
+        std::vector<std::string> key_value;
+        boost::algorithm::split(key_value, option, boost::is_any_of("="));


Careful — the value may actually have = signs in it, and there's no good way to quote. We should just use std::string::find to get the first = sign, and leave the rest alone.

igorpeshansky · 2018-03-30T18:15:51Z

+        }
+        input_str += key_value[0] + ": " + key_value[1] + "\n";
+      }
+      if (verbose_logging_) {


Let's use #ifdef VERBOSE. These are really debugging statements...

igorpeshansky · 2018-03-30T18:16:07Z

+      if (verbose_logging_) {
+        std::cout << "Options:\n" << input_str;
+      }
+      std::istringstream input { input_str };


The convention elsewhere in the code is to use parentheses.

outdated line

igorpeshansky · 2018-03-30T18:23:23Z

+          std::cerr << "Invalid option " << option;
+          return 1;
+        }
+        input_str += key_value[0] + ": " + key_value[1] + "\n";


Rather than construct one long string, how about creating an std::stringstream, using << to write to it, and then passing it into ParseConfiguration?

igorpeshansky

LGTM

igorpeshansky · 2018-03-30T19:10:10Z

-      std::string input_str;
+    ParseConfigFile(config_file);
+
+    // Command line options overwrite the options provided in the config file.


s/overwrite/override/

igorpeshansky · 2018-03-30T19:10:32Z

+
+    // Command line options overwrite the options provided in the config file.
+    if (flags.count("option")) {
+      std::stringstream input_stream;


Needs #include <sstream>.

igorpeshansky · 2018-03-30T19:59:12Z

 #include <boost/program_options.hpp>
 #include <iostream>
 #include <map>
 #include <fstream>


Let's alphabetize these while we're at it.

igorpeshansky · 2018-03-30T19:59:20Z


 #include "configuration.h"

+#include <boost/algorithm/string.hpp>


No longer needed, right?

correct. removed

igorpeshansky · 2018-03-30T19:59:39Z


 int Configuration::ParseArguments(int ac, char** av) {
  std::string config_file;
+  std::string command_line_options;


igorpeshansky · 2018-03-30T20:00:25Z

+
+    // Command line options override the options provided in the config file.
+    if (flags.count("option")) {
+      std::stringstream input_stream;


s/input_stream/option_stream/ — otherwise it looks weird that you're writing to it.

igorpeshansky

LGTM

rbuskens · 2018-03-31T13:31:12Z

LGTM

supriyagarg requested review from bmoyles0117 and igorpeshansky March 30, 2018 17:56

igorpeshansky suggested changes Mar 30, 2018

View reviewed changes

igorpeshansky approved these changes Mar 30, 2018

View reviewed changes

igorpeshansky requested review from rbuskens and removed request for bmoyles0117 March 30, 2018 19:11

igorpeshansky suggested changes Mar 30, 2018

View reviewed changes

supriyagarg added 4 commits March 30, 2018 16:30

Add support for providing command line options.

bfe756b

Address comments

f8ee1a9

Address comments 2

939a2db

Clean up unused imports and variables

210b5be

supriyagarg force-pushed the command-line-config branch from f7a958c to 210b5be Compare March 30, 2018 20:30

igorpeshansky approved these changes Mar 30, 2018

View reviewed changes

igorpeshansky requested review from bmoyles0117 and removed request for rbuskens March 31, 2018 04:05

igorpeshansky requested review from rbuskens and removed request for bmoyles0117 March 31, 2018 15:15

Whitespace

571afca

igorpeshansky merged commit c11744a into Stackdriver:master Mar 31, 2018


		#include "configuration.h"

		#include <boost/algorithm/string.hpp>

Conversation

supriyagarg commented Mar 30, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

supriyagarg Mar 30, 2018 • edited by igorpeshansky Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

igorpeshansky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

igorpeshansky left a comment

Choose a reason for hiding this comment

Uh oh!

rbuskens commented Mar 31, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

supriyagarg Mar 30, 2018 •

edited by igorpeshansky

Loading