Skip to content

Conversation

@hwh33
Copy link

@hwh33 hwh33 commented May 8, 2015

Implements a test server for the bundler and scan packages (addressing issues #117 and #96). This test server is flexible, so it should be usable for future packages as well. Also re-factors the existing CFSSL server.

@0xhaven 0xhaven self-assigned this May 8, 2015
@hwh33 hwh33 force-pushed the harry/scanserver branch from 6e2fcbe to 339e22a Compare May 8, 2015 07:46
Copy link
Contributor

Choose a reason for hiding this comment

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

this is sort of complicated. why not using initca.New(request) directly? That's much easier, right?

@lziest
Copy link
Contributor

lziest commented May 8, 2015

Also, it appears helpers/testsuite/test_certificates.go is defining helper functions that are not utilized. Separate it for a second commit or omit it for this pull request to avoid confusion.

Copy link
Contributor

Choose a reason for hiding this comment

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

comments or unused code?

@lziest
Copy link
Contributor

lziest commented May 8, 2015

Good job. Thanks.

@hwh33 hwh33 force-pushed the harry/scanserver branch from fd1d634 to 6860ce4 Compare May 9, 2015 08:13
@lziest
Copy link
Contributor

lziest commented May 11, 2015

LGTM

@lziest
Copy link
Contributor

lziest commented May 11, 2015

The commit message may need some cleanup though.

@hwh33 hwh33 force-pushed the harry/scanserver branch from 6860ce4 to 7307805 Compare May 11, 2015 17:06
@hwh33
Copy link
Author

hwh33 commented May 11, 2015

Ah yes, sorry about that. How is that? It's a bit verbose, but I added/changed a few distinct things and had trouble summarizing. Let me know if there's something you would prefer.

@lziest
Copy link
Contributor

lziest commented May 11, 2015

@hwh33 you can do multi-line commit message. The first line is the summary. And details can be itemized as "- add test servers", "- redo certificate generation" "- add tests to bundler and scan" etc. in the following lines.

@hwh33
Copy link
Author

hwh33 commented May 11, 2015

@lziest Oh okay, I see. I'll go ahead and change that. Thank you!

  - Add TestServer to helpers/testsuite for tests requiring a configurable, local TLS server
  - Remove CreateSelfSignedCert from helpers/testsuite due to overlap with initca.New
  - Complete re-do of SignCertificate in helpers/testsuite using internal packages (rather than parsing CLI)
  - Re-factor helpers/testsuite into logical distinct files for maintainability
  - Add tests to helpers/testsuite for TestServer
  - Add tests to bundler to test BundleFromRemote against local testsuite.TestServer (TLS)
  - Add tests to scan to test against local testsuite.TestServer (both TLS and TCP)
@hwh33 hwh33 force-pushed the harry/scanserver branch from 7307805 to 57a3be2 Compare May 11, 2015 17:34
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 it is ok not to test this. This is causing race condition in the tests and we probably never start two servers in the functional tests.

Copy link
Author

Choose a reason for hiding this comment

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

@lziest Okay, that makes sense. I removed the test.

  - Add TestServer to helpers/testsuite for tests requiring a configurable, local TLS server
  - Remove CreateSelfSignedCert from helpers/testsuite due to overlap with initca.New
  - Complete re-do of SignCertificate in helpers/testsuite using internal packages (rather than parsing CLI)
  - Re-factor helpers/testsuite into logical distinct files for maintainability
  - Add tests to helpers/testsuite for TestServer
  - Add tests to bundler to test BundleFromRemote against local testsuite.TestServer (TLS)
  - Add tests to scan to test against local testsuite.TestServer (both TLS and TCP)
@hwh33 hwh33 force-pushed the harry/scanserver branch from 45f5d58 to f15a07d Compare May 15, 2015 03:33
@grittygrease
Copy link
Contributor

@kisom @lziest What's the status of this PR? Are there issues in this code that are not resolved?

@0xhaven 0xhaven removed their assignment Oct 13, 2015
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.

4 participants