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

fix issue #6394: capture git stderr/stdout #414

Merged
merged 1 commit into from Aug 26, 2015

Conversation

asdil12
Copy link
Member

@asdil12 asdil12 commented Jul 29, 2015

Adds new dependency: perl(IPC::Run::Debug)

This patch is not tested, yet.

@coolo
Copy link
Contributor

coolo commented Jul 29, 2015

you don't seriously need another non-core perl module to catch stderr

@asdil12
Copy link
Member Author

asdil12 commented Jul 29, 2015

This module is already used by os-autoinst backends and does the job nicely.
So we do not need to package anything new.

@coolo
Copy link
Contributor

coolo commented Jul 29, 2015

I don't really care, but this sounds like a hundred times too complex

@coolo
Copy link
Contributor

coolo commented Jul 30, 2015

let us know when you tested it

@coolo
Copy link
Contributor

coolo commented Jul 31, 2015

I suggest you move these 4 lines you added 3 times into a wrapper function in Utils.pm. This way we can replace the solution later if we feel like it

@coolo
Copy link
Contributor

coolo commented Jul 31, 2015

and this way you should be able to write a test case for that util function easily - btw, the git errors shouldn't be ->debug but ->error IMO

@asdil12 asdil12 force-pushed the git_output branch 5 times, most recently from 8103b1e to 35d611a Compare August 24, 2015 11:59
@asdil12
Copy link
Member Author

asdil12 commented Aug 24, 2015

Done.

The test https://github.com/asdil12/openQA/blob/35d611a744781f4a028a9e803d7716947e8d5681/t/16-utils-runcmd.t runs successfully.
But I only test for the exit status - not for the logging itself as I don't have the $app object available while testing (and also I don't see a method in Mojo::Test to to asserts on the log output).

@@ -0,0 +1,30 @@
#!/usr/bin/env perl -w

# Copyright (C) 2014 SUSE Linux Products GmbH
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use correct copyright, which happen to be
Copyright (C) 2015 SUSE LLC

@asdil12
Copy link
Member Author

asdil12 commented Aug 24, 2015

Added correct copyright.

@@ -32,6 +32,7 @@ FindBin
Getopt::Long
IO::Socket::INET6
IO::Socket::SSL
IPC::Run::Debug
Copy link
Contributor

Choose a reason for hiding this comment

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

IPC::Run::Debug? Why not IPC::Run?

Copy link
Member Author

Choose a reason for hiding this comment

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

os-autoinst has IPC::Run::Debug in it's DEPENDENCIES.txt so I wanted to make sure that I use the same RPM.

But I guess the rpm should also provide IPC::Run - do you want me to change it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, didn't notice that. Anyway I think, requiring ::Debug variant and counting on that it will pull the base is not quite correct. (And on openSUSE IPC::Run::* is provided by the same package)

Adds new dependency: perl(IPC::Run)
@asdil12
Copy link
Member Author

asdil12 commented Aug 24, 2015

changed dependency to IPC::Run

aaannz pushed a commit that referenced this pull request Aug 26, 2015
fix issue #6394: capture git stderr/stdout
@aaannz aaannz merged commit f67ffa8 into os-autoinst:master Aug 26, 2015
@asdil12 asdil12 deleted the git_output branch September 7, 2015 13:18
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