Skip to content
This repository has been archived by the owner on Jun 28, 2024. It is now read-only.

Run collect script under ci #194

Merged

Conversation

jodh-intel
Copy link
Contributor

  • Run the data collection script to see if it works.

  • If the CI run fails, run the data collection script to help provide further clues on the failure.

Fixes #193.

Signed-off-by: James O. D. Hunt [email protected]

Run the data collection script to see if it works.

Partially fixes kata-containers#193.

Signed-off-by: James O. D. Hunt <[email protected]>
Add blank lines to make the teardown script clearer.

Signed-off-by: James O. D. Hunt <[email protected]>
If the CI run fails, run the data collection script to help provide
further clues on the failure.

Fixes kata-containers#193.

Signed-off-by: James O. D. Hunt <[email protected]>
@jodh-intel jodh-intel force-pushed the run-collect-script-under-ci branch from e1bfb09 to 962e27f Compare April 3, 2018 10:07
Copy link
Contributor

@grahamwhaley grahamwhaley left a comment

Choose a reason for hiding this comment

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

General principle lgtm - one query...

@@ -71,3 +71,6 @@ sudo -E PATH="$PATH" bash -c "make test"

echo "INFO: Checking log files"
check_log_files

echo "INFO: Running data collection script"
sudo -E PATH="$PATH" chronic kata-collect-data.sh
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you know what I'm going to say ... ;-)
Does this currently come out in the main CI log? Any chance you can redirect the output (at least stdout) to a known log file location - and then we can use that output in the teardown script, and save:

  • injecting the rather large collect script output into the main console log
  • having to run the script more than once?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Chronic discards all output unless the command fails so although there are 2 calls, this one won't bloat the console logs unless the script itself is broken (which we would want to know about).

Copy link
Contributor

Choose a reason for hiding this comment

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

ah, indeed, I didn't spot the chronic - mea culpa.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know what you're saying, but the two calls are for different reasons - this one basically checks the script "works" (very simplistically of course), whereas the other call captures the script output if a PR fails (implicitly expecting the script to work).

That teardown script is a bit fragile imho as it has to cater for two separate scenarios so I'd be a little bit leery of adding more complexity to it by getting it to find the output of this script and potentially chop it into bits, etc.

Copy link
Contributor

@grahamwhaley grahamwhaley left a comment

Choose a reason for hiding this comment

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

lgtm

@chavafg
Copy link
Contributor

chavafg commented Apr 3, 2018

lgtm

Approved with PullApprove

@chavafg chavafg merged commit 50ea1ca into kata-containers:master Apr 3, 2018
@chavafg chavafg removed the review label Apr 3, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants