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

Fix logging for SystemV packages #1022

Closed
wants to merge 1 commit into from
Closed

Fix logging for SystemV packages #1022

wants to merge 1 commit into from

Conversation

timcharper
Copy link
Contributor

The current implementation has no effect; nothing is logged. '>>' seems
to be passed to the process as a parameter and not something that is
interpretted by the shell.

This approach is in line with the accepted solution suggested at
https://stackoverflow.com/a/21029952/183863

Fixes #1021

@timcharper
Fix logging for SystemV packages
706eef5 
The current implementation has no effect; nothing is logged. '>>' seems
to be passed to the process as a parameter and not something that is
interpretted by the shell.

This approach is in line with the accepted solution suggested at
https://stackoverflow.com/a/21029952/183863

Fixes sbt#1021
@muuki88 muuki88 requested a review from kardapoltsev August 29, 2017 21:57
@muuki88 muuki88 added debian rpm and removed rpm labels Aug 29, 2017
muuki88
muuki88 requested changes Aug 30, 2017
View reviewed changes
Copy link
Contributor

@muuki88 muuki88 left a comment

Choose a reason for hiding this comment

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

The debian / sysvinit-deb init test is failing as we check for the startup line. Can you change this :)

@muuki88
Copy link
Contributor

muuki88 commented Aug 30, 2017

The failing MacOSX build can be ignored. Seems that travis doesn't like the scala language setting

@muuki88
Copy link
Contributor

muuki88 commented Sep 21, 2017

I'll be on vacation un 20.10. Thanks for your understanding if this can't be merged until then

@muuki88
Copy link
Contributor

muuki88 commented Nov 1, 2017

hi ho 😄 @timcharper after debian / sysvinit-deb is fixed, this is good to merge 👍

@dwickern
Copy link
Collaborator

dwickern commented Nov 8, 2017

I noticed this too. Thanks for the fix, I can confirm it works.

It's odd that daemonStdoutLogFile is defined for rpm but not deb. We should make them consistent

kardapoltsev
kardapoltsev reviewed Dec 24, 2017
View reviewed changes
Copy link
Member

@kardapoltsev kardapoltsev left a comment

Choose a reason for hiding this comment

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

  • 1 for merging after a fix for debian / sysvinit-deb.

@muuki88
Copy link
Contributor

muuki88 commented Dec 24, 2017

@kardapoltsev do you have the time to fix the test and merge this? I think easiest way is to open a new PR based on this one. 😍😍

@kardapoltsev
Copy link
Member

@kardapoltsev do you have the time to fix the test and merge this? I think easiest way is to open a new PR based on this one. 😍😍

Ok, will create new PR tomorrow.

@muuki88
Copy link
Contributor

muuki88 commented Dec 24, 2017

Awesome. Happy holidays 🤩

@muuki88
Copy link
Contributor

muuki88 commented Jan 8, 2018

ping @kardapoltsev 😉 You want to update this one?

@kardapoltsev kardapoltsev mentioned this pull request Jan 24, 2018
Merged
@muuki88
Copy link
Contributor

muuki88 commented Mar 30, 2018

Merged in #1096

@muuki88 muuki88 closed this Mar 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kardapoltsev kardapoltsev kardapoltsev left review comments

@muuki88 muuki88 muuki88 requested changes

Assignees
No one assigned
Labels
debian
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@timcharper @muuki88 @dwickern @kardapoltsev