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.

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

Fixes #12593: Add support for BSD-style init scripts services (rc.d) (slackware) #764

Conversation

victorqrt
Copy link
Contributor

@victorqrt victorqrt force-pushed the ust_12593/add_support_for_bsd_style_init_scripts_services_rc_d_slackware branch 5 times, most recently from f63011b to f21b2c7 Compare May 29, 2018 13:17
@victorqrt
Copy link
Contributor Author

Commit modified

@victorqrt victorqrt force-pushed the ust_12593/add_support_for_bsd_style_init_scripts_services_rc_d_slackware branch from f21b2c7 to 129a927 Compare May 29, 2018 13:18
"destination_exists" and => { "destination_defined", "destination_not_empty" };

methods:
"copy" usebundle => ncf_classes_copy("${source_prefix}", "${destination_prefix}");
Copy link
Member

Choose a reason for hiding this comment

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

missing indent

@@ -987,6 +1017,10 @@ bundle agent ncf_services(service, action)
"action_command" string => "${svc_action_cmd[${action}]}";
"method" string => "svcadm/svcs";

# Slackware
pass1.slackware.!systemctl_utility_present.!is_upstart_service.!svcadm_utility_present::
Copy link
Member

Choose a reason for hiding this comment

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

if it is a slackware the other conditions are useless

"action_command" string => "${paths.path[test]} -f /etc/rc`/sbin/runlevel | ${paths.path[cut]} -d' ' -f2`.d/S??${service}";
"method" string => "/etc/rcX.d/";

# Slackware boot actions are not performed by command, they are implemented in the 'methods' promises
Copy link
Member

Choose a reason for hiding this comment

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

this message doesn't seem to be at its place

Copy link
Contributor Author

@victorqrt victorqrt May 30, 2018

Choose a reason for hiding this comment

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

It is left here because in the previous "non-boot actions" block, we are defining action_commands and/or methods for all the platforms, so one may expect to find Slackware boot actions at this place. As we do not perform those actions in "vars" promises, this is explaining where to find them

ifvarclass => "!is_check_action";
# is-active is mapped to "status"
"action_command" string => "/etc/rc.d/rc.${service} status",
ifvarclass => "is_check_action";
Copy link
Member

Choose a reason for hiding this comment

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

this should have been merged with the previous block

Copy link
Member

Choose a reason for hiding this comment

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

moreover the "method" string line should be here for a better readability

@@ -1114,7 +1158,7 @@ bundle agent ncf_services(service, action)
#####
# Actual command - for checks on non-windows systems
#####
pass1.is_check_action.!windows.!is_process_action.method_found::
pass1.is_check_action.!windows.!is_process_action.method_found.!(slackware.is_boot_action)::
Copy link
Member

Choose a reason for hiding this comment

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

can is_check_action be defined at the same time as is_boot_action ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, iff action == "is-enabled"

"chmod 644" usebundle => perm_on_file("/etc/rc.d/rc.${service}", "644", "${old_class_prefix}", "${class_prefix}");

action_enable::
"chmod 755" usebundle => perm_on_file("/etc/rc.d/rc.${service}", "755", "${old_class_prefix}", "${class_prefix}");
Copy link
Member

Choose a reason for hiding this comment

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

idem

action_is_enabled::
"enable dry run" usebundle => push_dry_run_mode("true");
"chmod 755" usebundle => perm_on_file("/etc/rc.d/rc.${service}", "755", "${canon_service}_init_script_execmod", "${canon_service}_init_script_execmod");
"block in init scripts" usebundle => service_block_rc_d("${service}", "${old_class_prefix}", "${class_prefix}");
Copy link
Member

Choose a reason for hiding this comment

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

I think you need an ifvarclass ${canon_service}_init_script_execmod_ok here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can add it to force the bundle to stop checking on failing the first condition (755 mod on the init script).
It does work without it however, because in case it fails and then meets the "block in script" requirement, the _not_ok classes will still get reported.

files:
"/etc/rc.d/rc.local"
edit_line => edit_service_block_rc_d("${service}"),
classes => classes_generic("service_${canon_service}_block_in_rc_local");
Copy link
Member

Choose a reason for hiding this comment

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

So we prefer editing rc.local over editing rc.M ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes !

edit_line => edit_service_block_rc_d("${service}"),
classes => classes_generic("service_${canon_service}_block_in_rc_local");

"/etc/rc.d/rc.M"
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 add a line of comment to tell what happens when wry_run mode is enabled ?

ifvarclass => "ncf_services_${canonified_service_name}_is_enabled_reached";
"new result classes" usebundle => _classes_copy("${class_prefix}_check_enabled", "${class_prefix}"),
ifvarclass => "${class_prefix}_check_enabled_reached";
"success" usebundle => _classes_success("${old_class_prefix}"),
Copy link
Member

Choose a reason for hiding this comment

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

Why this change ? is there a repaired that appears during an is-enabled ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was for testing purposes, I forgot to add it back.

@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/ncf/pull/764
-- Your faithful QA

@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/ncf/pull/764
-- Your faithful QA

Copy link
Member

@peckpeck peckpeck left a comment

Choose a reason for hiding this comment

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

It was a mistake to approve it

@victorqrt
Copy link
Contributor Author

Commit modified

@victorqrt victorqrt force-pushed the ust_12593/add_support_for_bsd_style_init_scripts_services_rc_d_slackware branch from 129a927 to 8ce24f1 Compare June 5, 2018 08:49

# When the dry run mode is enabled, the needed changes will not be applied, but the classes will be copied
files:
"/etc/rc.d/rc.local"
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 manage rc.localshutdown too

@victorqrt
Copy link
Contributor Author

Commit modified

@victorqrt victorqrt force-pushed the ust_12593/add_support_for_bsd_style_init_scripts_services_rc_d_slackware branch from 8ce24f1 to 9f87087 Compare June 7, 2018 14:40
@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/ncf/pull/764
-- Your faithful QA

@peckpeck
Copy link
Member

OK, merging this PR

@peckpeck peckpeck merged commit 9f87087 into Normation:branches/rudder/4.1 Jun 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants