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 #11950: Add a rudder-server init script that will replace the rudder script on server #1449

Merged
merged 1 commit into from Jan 19, 2018
Merged

Fixes #11950: Add a rudder-server init script that will replace the rudder script on server #1449

merged 1 commit into from Jan 19, 2018

Conversation

amousset
Copy link
Member

@amousset amousset mentioned this pull request Jan 16, 2018
Closed
@amousset
Copy link
Member Author

PR rebased

@amousset amousset force-pushed the ust_11950/add_a_rudder_server_init_script_that_will_replace_the_rudder_script_on_server branch from 43d7ded to a84712d Compare January 18, 2018 08:17
@amousset
Copy link
Member Author

Commit modified

@amousset amousset force-pushed the ust_11950/add_a_rudder_server_init_script_that_will_replace_the_rudder_script_on_server branch from a84712d to 991cdd5 Compare January 18, 2018 09:49
@amousset
Copy link
Member Author

Commit modified

@amousset amousset force-pushed the ust_11950/add_a_rudder_server_init_script_that_will_replace_the_rudder_script_on_server branch from 991cdd5 to 33a8431 Compare January 18, 2018 10:00
peckpeck
peckpeck requested changes Jan 18, 2018
View reviewed changes
rudder-agent/SOURCES/rudder-agent-postinst Outdated
rm -f /var/rudder/tmp/migration-rudder-service-rename
rm -f /var/rudder/tmp/migration-rudder-service-systemd
rm -f /var/rudder/tmp/migration-rudder-service-enabled
rm -f /var/rudder/tmp/migration-rudder-cf-serverd-disabled
Copy link
Member

Choose a reason for hiding this comment

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

Why do you do this here ? They are used just afterwards

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, but if the rudder-server-root is installed after rudder-agent, we need to keep the flags (as the init file will be gone too)

Copy link
Member

Choose a reason for hiding this comment

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

how does removing them at the begining of the postinst prevent that ?

Copy link
Member Author

Choose a reason for hiding this comment

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

It will absolutely not, you're right, do you have an idea to handle this problem?

rudder-server-root/SOURCES/Makefile
true

install:
# Init files
Copy link
Member

Choose a reason for hiding this comment

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

Since this is a double negation, put a comment to say what it means for the default value

Copy link
Member

Choose a reason for hiding this comment

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

could be better, like "default is false -> use init"

rudder-server-root/SPECS/rudder-server-root.spec Outdated
echo "Rudder is now installed but not configured."
echo "Please run /opt/rudder/bin/rudder-init"
echo "************************************************************"
CFRUDDER_FIRST_INSTALL=0
Copy link
Member

Choose a reason for hiding this comment

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

CFRUDDER_FIRST_INSTALL=$1 ?

Copy link
Member Author

Choose a reason for hiding this comment

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

indeed!

rudder-server-root/debian/rules
@@ -49,7 +44,8 @@ binary-arch: install
dh_installchangelogs
# dh_installdocs
# dh_installexamples
dh_install --SOURCEDIR=$(CURDIR)/SOURCES/ rudder-server-root /opt/rudder/etc/server-roles.d/
Copy link
Member

Choose a reason for hiding this comment

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

why do you remove the roles file ?

Copy link
Member Author

Choose a reason for hiding this comment

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

install in now done in make install

@amousset
Copy link
Member Author

Commit modified

@amousset amousset force-pushed the ust_11950/add_a_rudder_server_init_script_that_will_replace_the_rudder_script_on_server branch 2 times, most recently from 77c807c to 54cb7c9 Compare January 19, 2018 14:21
@amousset
Copy link
Member Author

Commit modified

peckpeck
peckpeck reviewed Jan 19, 2018
View reviewed changes
rudder-agent/SPECS/rudder-agent.spec Outdated
@@ -358,13 +358,13 @@ then
# If old rudder-agent service is here and enabled
if chkconfig --list rudder-agent 2>&1 | grep -q -e 3:on -e B:on
then
touch /var/rudder/tmp/migration-rudder-service-enabled
touch /var/rudder/tmp/migration-rudder-service-enabled{,-server}
Copy link
Member

Choose a reason for hiding this comment

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

bashism

peckpeck
peckpeck reviewed Jan 19, 2018
View reviewed changes
rudder-agent/debian/preinst Outdated
@@ -59,12 +59,12 @@ then

if test /etc/rc`/sbin/runlevel | cut -d' ' -f2`.d/S??rudder-agent
then
touch /var/rudder/tmp/migration-rudder-service-enabled
touch /var/rudder/tmp/migration-rudder-service-enabled{,-server}
Copy link
Member

Choose a reason for hiding this comment

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

bashism

peckpeck
peckpeck reviewed Jan 19, 2018
View reviewed changes
rudder-server-root/SOURCES/rudder-server-root-postinst Outdated
fi

# Clean flag
rm -r /var/rudder/tmp/migration-rudder-service-enabled-server
Copy link
Member

Choose a reason for hiding this comment

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

did you mean -f ?

peckpeck
peckpeck reviewed Jan 19, 2018
View reviewed changes
rudder-server-root/SPECS/rudder-server-root.spec
then
if chkconfig --list rudder 2>&1 | grep -q -e 3:on -e B:on
then
touch /var/rudder/tmp/migration-rudder-service-enabled-server
Copy link
Member

Choose a reason for hiding this comment

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

missing the 2nd file

Copy link
Member Author

Choose a reason for hiding this comment

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

We do not need it here as rudder-server-root scripts have no side effect on rudder-agent

peckpeck
peckpeck reviewed Jan 19, 2018
View reviewed changes
rudder-server-root/debian/preinst
then
if test /etc/rc`/sbin/runlevel | cut -d' ' -f2`.d/S??rudder
then
touch /var/rudder/tmp/migration-rudder-service-enabled-server
Copy link
Member

Choose a reason for hiding this comment

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

missing the second file ?

peckpeck
peckpeck reviewed Jan 19, 2018
View reviewed changes
rudder-agent/debian/rules Outdated
@@ -62,7 +62,7 @@ install: build
dh_installdirs
cd SOURCES && $(MAKE) install $(MAKE_OPTIONS) DESTDIR=$(CURDIR)/debian/tmp
# let this file be managed by dh_installinit
mv $(CURDIR)/debian/tmp/etc/init.d/rudder-agent debian/rudder-agent.init
mv $(CURDIR)/debian/tmp/etc/init.d/rudder-server.init debian/rudder-server.init
Copy link
Member

Choose a reason for hiding this comment

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

??

@amousset
Copy link
Member Author

Commit modified

@amousset amousset force-pushed the ust_11950/add_a_rudder_server_init_script_that_will_replace_the_rudder_script_on_server branch from 54cb7c9 to e1e60ab Compare January 19, 2018 16:11
@amousset
Copy link
Member Author

Commit modified

@amousset amousset force-pushed the ust_11950/add_a_rudder_server_init_script_that_will_replace_the_rudder_script_on_server branch from e1e60ab to 1bb7be6 Compare January 19, 2018 16:12
@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-packages/pull/1449
-- Your faithful QA

@amousset
Copy link
Member Author

OK, merging this PR

@amousset amousset merged commit 1bb7be6 into Normation:branches/rudder/4.3 Jan 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@peckpeck peckpeck peckpeck requested changes

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