-
Notifications
You must be signed in to change notification settings - Fork 90
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
Fix appPrepConfigRunCmd to report stderr and status #483
Conversation
2> /opt/guestconfig/configure.stderr &` | ||
echo -n %s= > ` + appPrepConfigStatus + ` && | ||
nohup sh -c "` + appPrepStartscript + ` --configure 2>/opt/guestconfig/configure.stderr 1>/opt/guestconfig/configure.stdout; | ||
echo $? >> ` + appPrepConfigStatus + `" &` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was removing the "-n" option here intentional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think it's not required since line 92 has -n, so finally configure.status file looks like
docker://789aabbca73481a9b0b2f0891281cd32e05995353287a9e1b07810bcf6d1417c=0
which is what we need
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, but previously we didn't have a trailing carriage return after all that. It may not matter, depends on the processing code, but just wondering why you put that final carriage return back in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On line 93 ?
For readability, it's basically split into 3 subcommands
line 92 - write container id in status
line 93 - run startscript in the background and write stdout and stderr in respective files
line 94 - update status file with exit code of the previous command.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it's the change in line 94 that I'm asking about.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, I see this is the original question of why did we remove "-n". Actually it is not making any diff in status. I always see docker://789aabbca73481a9b0b2f0891281cd32e05995353287a9e1b07810bcf6d1417c=0
We can add -n back.
But there is a new problem that I found. With nohup the $? is always returning 0.
Trying to debug that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added -n back, and replaced double quote with a single.
This now works as expected
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we sure. :-)
Man this particular code has been ornery.
Minor tweaks to appPrepConfigRunCmd to fix stderr and status which was getting lost.
Even a failed startscript was not writing anything to /opt/guesstconfig/configure.stderr or /opt/guesstconfig/configure.status.
Tested all /opt/guestconfig/configure.* for successful and unssucessful runs of startscript.