Rename MetadataAgentConfiguration to Configuration by ACEmilG · Pull Request #91 · Stackdriver/metadata-agent

ACEmilG · 2018-03-26T15:12:48Z

No description provided.

igorpeshansky

I'm sure you've seen this branch. 😄
Some stylistic comments...

igorpeshansky · 2018-03-26T18:23:34Z

src/api_server.h

   public:
-    Handler(const MetadataAgentConfiguration& config,
+    Handler(const Configuration& config,
            const MetadataStore& store);


Fits on one line now.

igorpeshansky · 2018-03-26T18:28:47Z

src/docker.h

 public:
-  DockerUpdater(const MetadataAgentConfiguration& config,
+  DockerUpdater(const Configuration& config,
                MetadataStore* store)


Fits on one line now.

igorpeshansky · 2018-03-26T18:28:59Z

src/instance.h

 public:
-  InstanceUpdater(const MetadataAgentConfiguration& config,
+  InstanceUpdater(const Configuration& config,
                  MetadataStore* store)


Fits on one line now.

igorpeshansky · 2018-03-26T18:29:17Z

src/kubernetes.h

 public:
-  KubernetesUpdater(const MetadataAgentConfiguration& config,
+  KubernetesUpdater(const Configuration& config,
                    MetadataStore* store);


Fits on one line now.

igorpeshansky · 2018-03-26T18:29:54Z

src/store.cc


-MetadataStore::MetadataStore(const MetadataAgentConfiguration& config)
+MetadataStore::MetadataStore(const Configuration& config)
    : config_(config) {}


This can fit on the previous line now.

igorpeshansky · 2018-03-26T18:31:09Z

src/updater.h


-  MetadataUpdater(const MetadataAgentConfiguration& config,
+  MetadataUpdater(const Configuration& config,
                  MetadataStore* store, const std::string& name);


MetadataStore* store, can fit on the previous line now.

bmoyles0117

LGTM

igorpeshansky · 2018-03-27T23:16:05Z

@ACEmilG Need to resolve the conflicts.

ACEmilG · 2018-03-28T15:24:13Z

@igorpeshansky: Resolved conflicts.

igorpeshansky

A few stragglers.

igorpeshansky · 2018-03-28T15:42:20Z

src/configuration.cc

-MetadataAgentConfiguration::MetadataAgentConfiguration(std::istream& input)
-    : MetadataAgentConfiguration() {
+Configuration::Configuration(std::istream& input)
+    : Configuration() {


Fits on the previous line now.

igorpeshansky · 2018-03-28T15:42:39Z

src/configuration.h

-  MetadataAgentConfiguration(std::istream&& input)
-      : MetadataAgentConfiguration(input) {}
+  Configuration(std::istream&& input)
+      : Configuration(input) {}


Fits on the previous line now.

igorpeshansky · 2018-03-28T15:45:48Z

src/reporter.h

 public:
-  MetadataReporter(const MetadataAgentConfiguration& config,
+  MetadataReporter(const Configuration& config,
                   MetadataStore* store, double period_s);


Same comment as in updater.h:
MetadataStore* store, can fit on the previous line now.

igorpeshansky · 2018-03-28T15:47:04Z

test/configuration_unittest.cc

 namespace google {

-void VerifyDefaultConfig(const MetadataAgentConfiguration& config) {
+void VerifyDefaultConfig(const Configuration& config) {


[Optional] Not directly related to this PR, but this could have been in an anonymous namespace...

I'll skip this for now, but deserves to get cleaned up at some later point.

igorpeshansky

One last comment.

igorpeshansky · 2018-03-28T16:08:08Z

src/api_server.h

 public:
-  MetadataApiServer(const MetadataAgentConfiguration& config,
+  MetadataApiServer(const Configuration& config,
                    const MetadataStore& store, int server_threads,


Sorry, didn't notice this before. Same here: const MetadataStore& store fits on the previous line. In fact, this can become:

MetadataApiServer(const Configuration& config, const MetadataStore& store, int server_threads, const std::string& host, int port);

igorpeshansky

LGTM

igorpeshansky suggested changes Mar 26, 2018

View reviewed changes

bmoyles0117 approved these changes Mar 27, 2018

View reviewed changes

ACEmilG force-pushed the ACEmilG-configuration-rename branch from 78986e4 to 997d92a Compare March 28, 2018 15:09

ACEmilG added 2 commits March 28, 2018 11:23

Rename MetadataAgentConfiguration to Configuration

8cf2500

Addressed style comments

2290d61

ACEmilG force-pushed the ACEmilG-configuration-rename branch from 997d92a to 2290d61 Compare March 28, 2018 15:23

igorpeshansky suggested changes Mar 28, 2018

View reviewed changes

Addressed code review comments

9155b2f

igorpeshansky suggested changes Mar 28, 2018

View reviewed changes

Style change

be321ff

igorpeshansky approved these changes Mar 28, 2018

View reviewed changes

igorpeshansky merged commit f7f6c42 into master Mar 28, 2018

igorpeshansky deleted the ACEmilG-configuration-rename branch March 28, 2018 17:02

Conversation

ACEmilG commented Mar 26, 2018

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!

bmoyles0117 left a comment

Choose a reason for hiding this comment

Uh oh!

igorpeshansky commented Mar 27, 2018

Uh oh!

ACEmilG commented Mar 28, 2018

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!

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!

igorpeshansky left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants