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

Add pgctl environ to log pipeline #180

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

yalinhuang
Copy link
Contributor

Add PGCTL_* environment variables to processes spun up by pgctl. This makes it easier for process management.

@bukzor
Copy link
Contributor

bukzor commented Feb 21, 2017

:shipit:

@@ -871,3 +872,42 @@ def it_runs_after_all_services_have_stopped(self):
0,
norm=norm.pgctl,
)


class DescribePGCTLEnvironment(object):
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: uppercasing pgctl feels a little odd.

@@ -176,19 +176,20 @@ def svstat_parse(svstat_string):
return SvStat(state, pid, exitcode, seconds, process)


def prepend_timestamps_to(logfile):
def prepend_timestamps_to(logfile, env=None):
Copy link
Contributor

Choose a reason for hiding this comment

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

Why default env?

return timestamps.stdin


def _pipeline(cmd, stdin, stdout):
def _pipeline(cmd, stdin, stdout, env=None):
Copy link
Contributor

Choose a reason for hiding this comment

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

Default again?

)
assert returncode == 0
assert stderr == ''
for pid in stdout.split('\n'):
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if lsof returns nothing?

@yalinhuang
Copy link
Contributor Author

After talking to bukzor in person, we will put a hold on this PR. We are able to resolve our issue in another (& better) solution. We will come back to this in the future if needs arise.

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.

3 participants