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

init.d script -- fix issue #3803 #3804

Merged
merged 1 commit into from
Aug 24, 2015
Merged

init.d script -- fix issue #3803 #3804

merged 1 commit into from
Aug 24, 2015

Conversation

KoeSystems
Copy link
Contributor

This bug was introduced in PR 3115.

@@ -45,7 +45,7 @@ PIDFILE=/var/run/influxdb/influxd.pid
PIDDIR=`dirname $PIDFILE`
if [ ! -d "$PIDDIR" ]; then
mkdir -p $PIDDIR
chown $GROUP:$USER $PIDDIR
chown $USER:$GROUP $PIDDIR
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks to @dgnorton in PR 3115 I realise there was a bug here.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 on this specific change.

@toddboom
Copy link
Contributor

Well, init scripts aren't my forte, but this all seems to make sense to me. What do you think, @otoolep?

@otoolep otoolep changed the title Fix issue #3803 init.d script -- fix issue #3803 Aug 24, 2015
@@ -62,6 +62,7 @@ fi

if [ ! -f "$STDOUT" ]; then
mkdir -p $(dirname $STDOUT)
chown $USER:$GROUP $(dirname $STDOUT)
Copy link
Contributor

Choose a reason for hiding this comment

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

Surely not. If STDOUT (and STDERR) are not changed from the defaults, this is changing the owner and group of /dev.

Copy link
Contributor

Choose a reason for hiding this comment

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

I.e. it makes no sense to do this, right?

chown influxdb:influxdb /dev

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Opps, that's true, I was trying to fix the corner case when you change from the default user influxdb to another user and the file/directory already exist.

I will revert this change.

@otoolep
Copy link
Contributor

otoolep commented Aug 24, 2015

2 of the changes make sense. The other 2 scare me!

@otoolep
Copy link
Contributor

otoolep commented Aug 24, 2015

Please let me know if I am missing something here.

@KoeSystems
Copy link
Contributor Author

@otoolep @toddboom It seems there is something wrong with the CI test.

github.com/influxdb/influxdb/uuid   [no test files] bash circle-test.sh returned exit code 1

You can see the same error in other PR

@otoolep
Copy link
Contributor

otoolep commented Aug 24, 2015

Thanks @KoeSystems -- can you squash the 3 changes into 1?

@otoolep otoolep closed this Aug 24, 2015
@otoolep otoolep reopened this Aug 24, 2015
@otoolep
Copy link
Contributor

otoolep commented Aug 24, 2015

Closed in error.

@otoolep
Copy link
Contributor

otoolep commented Aug 24, 2015

Thanks @KoeSystems -- merging now.

otoolep added a commit that referenced this pull request Aug 24, 2015
@otoolep otoolep merged commit 83577ba into influxdata:master Aug 24, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants