Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fixes #13983: Remove template in system techniques #1385

Merged
merged 1 commit into from
Dec 18, 2018
Merged

Fixes #13983: Remove template in system techniques #1385

merged 1 commit into from
Dec 18, 2018

Conversation

peckpeck
Copy link
Member

ncharles
ncharles reviewed Dec 14, 2018
View reviewed changes
techniques/system/server-roles/1.0/common.st Outdated
@@ -0,0 +1,6 @@
# Common variable for server-role system technique
bundle common roles_common {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm not fancying this name - it is not so clear.
maybe server_roles_common ?
also, you are missing the license

ncharles
ncharles reviewed Dec 14, 2018
View reviewed changes
techniques/system/server-roles/1.0/metadata.xml Outdated
@@ -36,6 +28,33 @@ along with this program. If not, see <http://www.gnu.org/licenses/>.
</TMLS>

<FILES>
<FILE name="component-check">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you are missing the file extension

@peckpeck
Copy link
Member Author

Commit modified

@peckpeck peckpeck force-pushed the arch_13983/remove_template_in_system_techniques branch from 1d8a4ce to 10f5c1a Compare December 17, 2018 09:49
ncharles
ncharles requested changes Dec 17, 2018
View reviewed changes
Copy link
Member

@ncharles ncharles left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there are some necessary changes

techniques/system/common/1.0/common.st
"rudder_configuration_repository"
string => "&CONFIGURATION_REPOSITORY_FOLDER&";
"heartbeat_interval" string => "&RUDDER_HEARTBEAT_INTERVAL&";
"rudder_node_config_id" string => "&RUDDER_NODE_CONFIG_ID&";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you align them all ? Or at least add a bit of consistency (davuser and davpw could be aligned with uuid)

techniques/system/common/1.0/metadata.xml
@@ -19,13 +19,8 @@ along with this program. If not, see <http://www.gnu.org/licenses/>.
<SYSTEM>true</SYSTEM>

<TMLS>
<TML name="monitoring" />
<TML name="cf-serverd"/>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you need to add common here

techniques/system/common/1.0/update.cf Outdated
@@ -38,7 +38,8 @@ body copy_from remote(server, path)
purge => "true";
copy_backup => "false";
community_edition::
portnumber => "&COMMUNITYPORT&";
# TODO why isn't this a constant ?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you remove this todo ?

techniques/system/distributePolicy/1.0/common.st Outdated
"relay_sync_method" string => "&RELAY_SYNC_METHOD&";
"relay_sync_promises" string => "&RELAY_SYNC_PROMISES&";
"relay_sync_sharedfiles" string => "&RELAY_SYNC_SHAREDFILES&";
"cmdb_endpoint" string => "&CMDBENDPOINT&";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you also align these ?

techniques/system/server-roles/1.0/alive-check.cf Outdated
@@ -59,18 +59,18 @@ bundle agent generic_alive_check(app)

pass3.root_server::

"any" usebundle => rudder_common_report("server-roles", "result_success", "&TRACKINGKEY&", "Check ${app} status", "None", "The ${site_to_check} web interface is running"),
"any" usebundle => rudder_common_report("server-roles", "result_success", "${roles_common.directiveId}", "Check ${app} status", "None", "The ${site_to_check} web interface is running"),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it should be rudder_roles_common

techniques/system/server-roles/1.0/component-check.cf Outdated
@@ -71,13 +71,13 @@ bundle agent root_component_check
"any" usebundle => root_password_restart_jetty;

!role_rudder_server_root::
"any" usebundle => rudder_common_report("${technique_name}", "result_na", "&TRACKINGKEY&",
"any" usebundle => rudder_common_report("${technique_name}", "result_na", "${roles_common.directiveId}",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it should be rudder_roles_common

techniques/system/server-roles/1.0/common.st Outdated
@@ -0,0 +1,6 @@
# Common variable for server-role system technique
bundle common rudder_roles_common {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you used roles_common everywhere, can you pick either one ?

@peckpeck
Copy link
Member Author

Commit modified

ncharles
ncharles requested changes Dec 18, 2018
View reviewed changes
Copy link
Member

@ncharles ncharles left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two minor remarks, but this is great !

techniques/system/common/1.0/common.st Outdated
"davuser" string => "&DAVUSER&";
"davpw" string => "&DAVPASSWORD&";
"rudder_configuration_repository"
string => "&CONFIGURATION_REPOSITORY_FOLDER&";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not aligning this one ? :(

techniques/system/common/1.0/cron-setup.st
@@ -110,10 +110,10 @@ bundle agent check_cron_daemon
"any" usebundle => _classes_success("service_restart_${service_name}");

pass3.!aix::
"any" usebundle => rudder_common_reports_generic("Common", "service_restart_${service_name}", "&TRACKINGKEY&", "CRON Daemon", "None", "Cron daemon status");
"any" usebundle => rudder_common_reports_generic("Common", "service_restart_${service_name}", "${system_common.directiveId}", "CRON Daemon", "None", "Cron daemon status");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is it still a .st ?
if you replace it with a .cf, you ought to replace & by &

@peckpeck
Copy link
Member Author

Commit modified

@ncharles
Copy link
Member

This is awesome !!! Thank you

@Normation-Quality-Assistant
Copy link
Contributor

This PR is not mergeable to upper versions.
Since it is "Ready for merge" you must merge it by yourself using the following command:
rudder-dev merge https://github.com/Normation/rudder-techniques/pull/1385
-- Your faithful QA
Kant merge: "Happiness is not an ideal of reason, but of imagination."
(https://ci.normation.com/jenkins/job/merge-accepted-pr/5235/console)

@peckpeck
Copy link
Member Author

OK, squash merging this PR

@peckpeck peckpeck force-pushed the arch_13983/remove_template_in_system_techniques branch from 6cb767d to 8c83e75 Compare December 18, 2018 15:14
@peckpeck
Copy link
Member Author

OK, merging this PR

@peckpeck peckpeck merged commit 8c83e75 into Normation:master Dec 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ncharles ncharles ncharles approved these changes

Assignees
No one assigned
Labels
qa: Can't merge
Projects
None yet
Milestone
No milestone
3 participants
@peckpeck @ncharles @Normation-Quality-Assistant