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

Enables PKB to run on a windows controller #274

Merged
merged 19 commits into from
May 27, 2015
Merged

Enables PKB to run on a windows controller #274

merged 19 commits into from
May 27, 2015

Conversation

ehankland
Copy link
Contributor

No description provided.

@ehankland
Copy link
Contributor Author

test this please.

1 similar comment
@ehankland
Copy link
Contributor Author

test this please.

@ehankland
Copy link
Contributor Author

test this please.

@@ -71,6 +71,34 @@ Before you can run the PerfKit Benchmaker on Cloud providers you need accounts a
You also need the software dependencies, which are mostly command line tools and credentials to access your
accounts without a password. The following steps should help you get the CLI tool auth in place.

If you are running on Windows, you will need to install Github Windows
Copy link
Contributor

Choose a reason for hiding this comment

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

Will a windows controller work for linux guests? If not, we should call that out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes a windows controller can run benchmarks on linux guests - that's all this PR enables - windows guests will come later.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah - okay, thanks.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: GitHub

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@cmccoy
Copy link
Contributor

cmccoy commented May 26, 2015

Looks good. Some small comments.

Given that Windows requires some additional prerequisites to be installed, I wonder if we should do a pre-run check for the presence of:

  • ssh
  • ssh-keygen
  • scp
  • openssl

@ehankland
Copy link
Contributor Author

PTAL

shell=shell_value,
stdout=subprocess.PIPE,
stderr=subprocess.PIPE)
process.wait()
Copy link
Contributor

Choose a reason for hiding this comment

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

Unlikely given the commands used, but this risks deadlock if cmd produces a lot of output. Prefer check_output, communicate, or opening a handle to os.devnull and using it for stdout and stderr.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@ehankland ehankland assigned cmccoy and unassigned ehankland May 27, 2015
@cmccoy cmccoy assigned ehankland and unassigned cmccoy May 27, 2015
@cmccoy cmccoy added the LGTM label May 27, 2015
ehankland added a commit that referenced this pull request May 27, 2015
Enables PKB to run on a windows controller
@ehankland ehankland merged commit ae7b6c7 into dev May 27, 2015
@ehankland ehankland deleted the windows branch May 27, 2015 22:02
@voellm voellm mentioned this pull request Jun 1, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants