Fix style and logging from review; enhance output

refs #4508

doc/06-distributed-monitoring.md

@@ -236,7 +236,9 @@ Enabling feature api. Make sure to restart Icinga 2 for these changes to take ef
 
 Master zone name [master]:
 
+Default global zones: global-templates director-global
 Do you want to specify additional global zones? [y/N]: N
+
 Please specify the API bind host/port (optional):
 Bind Host []:
 Bind Port []:
@@ -548,6 +550,7 @@ Press `Enter` or choose `n`, if you don't want to add any additional.
 ```
 Reconfiguring Icinga...
 
+Default global zones: global-templates director-global
 Do you want to specify additional global zones? [y/N]: N
 ```
 

lib/cli/apisetuputility.cpp

@@ -43,6 +43,11 @@ String ApiSetupUtility::GetConfdPath()
 	return Application::GetSysconfDir() + "/icinga2/conf.d";
 }
 
+String ApiSetupUtility::GetApiUsersConfPath()
+{
+	return ApiSetupUtility::GetConfdPath() + "/api-users.conf";
+}
+
 bool ApiSetupUtility::SetupMaster(const String& cn, bool prompt_restart)
 {
 	if (!SetupMasterCertificates(cn))

lib/cli/apisetuputility.hpp

@@ -45,6 +45,7 @@ public:
 	static bool SetupMasterUpdateConstants(const String& cn);
 
 	static String GetConfdPath();
+	static String GetApiUsersConfPath();
 
 private:
 	ApiSetupUtility();

lib/cli/nodesetupcommand.cpp

@@ -66,7 +66,7 @@ void NodeSetupCommand::InitParameters(boost::program_options::options_descriptio
 		("accept-config", "Accept config from master")
 		("accept-commands", "Accept commands from master")
 		("master", "Use setup for a master instance")
-		("global_zones", po::value<std::vector<std::string> >(), "The names of the additional global zones.")
+		("global_zones", po::value<std::vector<std::string> >(), "The names of the additional global zones to 'global-templates' and 'director-global'.")
 		("disable-confd", "Disables the conf.d directory during the setup");
 
 	hiddenDesc.add_options()
@@ -247,23 +247,24 @@ int NodeSetupCommand::SetupMaster(const boost::program_options::variables_map& v
 
 	if (vm.count("disable-confd")) {
 		/* Disable conf.d inclusion */
-		if (NodeUtility::UpdateConfiguration("\"conf.d\"", false, true))
+		if (NodeUtility::UpdateConfiguration("\"conf.d\"", false, true)) {
 			Log(LogInformation, "cli")
 				<< "Disabled conf.d inclusion";
-		else
+		} else {
 			Log(LogWarning, "cli")
 				<< "Tried to disable conf.d inclusion but failed, possibly it's already disabled.";
-
+		}
-		String apiUsersFilePath = Application::GetSysconfDir() + "/icinga2/conf.d/api-users.conf";
-		std::ifstream apiUsersFile(apiUsersFilePath);
 
 		/* Include api-users.conf */
-		if(apiUsersFile)
+		String apiUsersFilePath = ApiSetupUtility::GetApiUsersConfPath();
+
+		if (Utility::PathExists(apiUsersFilePath)) {
 			NodeUtility::UpdateConfiguration("\"conf.d/api-users.conf\"", true, false);
-		else
+		} else {
 			Log(LogWarning, "cli")
 				<< "Included file dosen't exist " << apiUsersFilePath;
 		}
+	}
 
 	/* tell the user to reload icinga2 */
 	Log(LogInformation, "cli", "Make sure to restart Icinga 2.");
@@ -576,17 +577,8 @@ int NodeSetupCommand::SetupNode(const boost::program_options::variables_map& vm,
 	}
 
 	if (vm.count("disable-confd")) {
-
 		/* Disable conf.d inclusion */
 		NodeUtility::UpdateConfiguration("\"conf.d\"", false, true);
-
-		String apiUsersFilePath = Application::GetSysconfDir() + "/icinga2/conf.d/api-users.conf";
-		std::ifstream apiUsersFile(apiUsersFilePath);
-
-		if(apiUsersFile)
-			NodeUtility::UpdateConfiguration("\"conf.d/api-users.conf\"", true, false);
-		else
-			Log(LogWarning, "cli", "Included file dosen't exist " + apiUsersFilePath);
 	}
 
 	/* tell the user to reload icinga2 */

lib/cli/nodeutility.cpp

@@ -272,7 +272,7 @@ void NodeUtility::SerializeObject(std::ostream& fp, const Dictionary::Ptr& objec
  * recursive = true, will search for a resursive include statement
  * Returns true on success, false if option was not found
  */
-bool NodeUtility::UpdateConfiguration(const String& value, const bool& include, const bool& recursive)
+bool NodeUtility::UpdateConfiguration(const String& value, bool include, bool recursive)
 {
 	String configurationFile = Application::GetSysconfDir() + "/icinga2/icinga2.conf";
 
@@ -287,7 +287,10 @@ bool NodeUtility::UpdateConfiguration(const String& value, const bool& include,
 
 	String affectedInclude = value;
 
-	recursive ? affectedInclude = "include_recursive " + affectedInclude : affectedInclude = "include " + affectedInclude;
+	if (recursive)
+		affectedInclude = "include_recursive " + affectedInclude;
+	else
+		affectedInclude = "include " + affectedInclude;
 
 	bool found = false;
 
@@ -297,7 +300,9 @@ bool NodeUtility::UpdateConfiguration(const String& value, const bool& include,
 		if (include) {
 			if (line.find("//" + affectedInclude) != std::string::npos || line.find("// " + affectedInclude) != std::string::npos) {
 				found = true;
-				ofp << affectedInclude + "\n";
+				ofp << "// Added by the node setup CLI command on "
+					<< Utility::FormatDateTime("%Y-%m-%d %H:%M:%S %z", Utility::GetTime())
+					<< "\n" + affectedInclude + "\n";
 			} else if (line.find(affectedInclude) != std::string::npos) {
 				found = true;
 
@@ -305,19 +310,26 @@ bool NodeUtility::UpdateConfiguration(const String& value, const bool& include,
 					<< "Include statement '" + affectedInclude + "' already set.";
 
 				ofp << line << "\n";
-			} else
+			} else {
 				ofp << line << "\n";
+			}
 		} else {
 			if (line.find(affectedInclude) != std::string::npos) {
 				found = true;
-				ofp << "// " + affectedInclude + "\n";
+				ofp << "// Disabled by the node setup CLI command on "
-			} else
+					<< Utility::FormatDateTime("%Y-%m-%d %H:%M:%S %z", Utility::GetTime())
+					<< "\n// " + affectedInclude + "\n";
+			} else {
 				ofp << line << "\n";
 			}
 		}
+	}
 
-	if (include && !found)
+	if (include && !found) {
-		ofp << affectedInclude + "\n";
+		ofp << "// Added by the node setup CLI command on "
+			<< Utility::FormatDateTime("%Y-%m-%d %H:%M:%S %z", Utility::GetTime())
+			<< "\n" + affectedInclude + "\n";
+	}
 
 	ifp.close();
 	ofp.close();

lib/cli/nodeutility.hpp

@@ -44,7 +44,7 @@ public:
 
 	static bool WriteNodeConfigObjects(const String& filename, const Array::Ptr& objects);
 
-	static bool UpdateConfiguration(const String& value, const bool& include, const bool& recursive);
+	static bool UpdateConfiguration(const String& value, bool include, bool recursive);
 	static void UpdateConstant(const String& name, const String& value);
 
 	/* node setup helpers */

lib/cli/nodewizardcommand.cpp

@@ -527,6 +527,7 @@ wizard_ticket:
 	/* Global zones. */
 	std::vector<String> globalZones { "global-templates", "director-global" };
 
+	std::cout << "\nDefault global zones: " << boost::algorithm::join(globalZones, " ");
 	std::cout << "\nDo you want to specify additional global zones? [y/N]: ";
 
 	std::getline(std::cin, answer);
@@ -625,7 +626,7 @@ wizard_global_zone_loop_start:
 
 	if (choice.Contains("n"))
 		Log(LogInformation, "cli")
-			<< "The deactivation of the conf.d directory was skipped.";
+			<< "conf.d directory has not been disabled.";
 	else {
 		std::cout << ConsoleColorTag(Console_Bold | Console_ForegroundGreen)
 			<< "Disabling the inclusion of the conf.d directory...\n"
@@ -633,10 +634,13 @@ wizard_global_zone_loop_start:
 
 		if (!NodeUtility::UpdateConfiguration("\"conf.d\"", false, true)) {
 			std::cout << ConsoleColorTag(Console_Bold | Console_ForegroundRed)
-				<< "Failed to disable conf.d inclusion, it may already be disabled.\n"
+				<< "Failed to disable the conf.d inclusion, it may already have been disabled.\n"
 				<< ConsoleColorTag(Console_Normal);
 		}
 
+		/* Satellite/Clients should not include the api-users.conf file.
+		 * The configuration should instead be managed via config sync or automation tools.
+		 */
 	}
 
 	return 0;
@@ -707,6 +711,7 @@ int NodeWizardCommand::MasterSetup() const
 	/* Global zones. */
 	std::vector<String> globalZones { "global-templates", "director-global" };
 
+	std::cout << "\nDefault global zones: " << boost::algorithm::join(globalZones, " ");
 	std::cout << "\nDo you want to specify additional global zones? [y/N]: ";
 
 	std::getline(std::cin, answer);
@@ -832,34 +837,32 @@ wizard_global_zone_loop_start:
 
 	if (choice.Contains("n"))
 		Log(LogInformation, "cli")
-			<< "The deactivation of the conf.d directory was skipped.";
+			<< "conf.d directory has not been disabled.";
 	else {
 		std::cout << ConsoleColorTag(Console_Bold | Console_ForegroundGreen)
-			<< "Disable the inclusion of the conf.d directory...\n"
+			<< "Disabling the inclusion of the conf.d directory...\n"
 			<< ConsoleColorTag(Console_Normal);
 
 		if (!NodeUtility::UpdateConfiguration("\"conf.d\"", false, true)) {
 			std::cout << ConsoleColorTag(Console_Bold | Console_ForegroundRed)
-				<< "Failed to disable conf.d inclusion, it may already be disabled.\n"
+				<< "Failed to disable the conf.d inclusion, it may already have been disabled.\n"
 				<< ConsoleColorTag(Console_Normal);
 		}
 
 		/* Include api-users.conf */
 		String apiUsersFilePath = Application::GetSysconfDir() + "/icinga2/conf.d/api-users.conf";
-		std::ifstream apiUsersFile(apiUsersFilePath);
 
 		std::cout << ConsoleColorTag(Console_Bold | Console_ForegroundGreen)
-			<< "Checking if api-users.conf exist...\n"
+			<< "Checking if the api-users.conf file exists...\n"
 			<< ConsoleColorTag(Console_Normal);
 
-				if(apiUsersFile)
+		if (Utility::PathExists(apiUsersFilePath)) {
 			NodeUtility::UpdateConfiguration("\"conf.d/api-users.conf\"", true, false);
-				else
+		} else {
 			Log(LogWarning, "cli")
-						<< "Included file dosen't exist " << apiUsersFilePath;
+				<< "Included file '" << apiUsersFilePath << "' does not exist.";
+		}
 	}
-
-	std::cout << "Done.\n\n";
 
 	return 0;
 }