Skip to content

vendor: latest tilt-apiserver and default to TLS connections#4417

Merged
nicks merged 1 commit into
masterfrom
nicks/vendor
Apr 12, 2021
Merged

vendor: latest tilt-apiserver and default to TLS connections#4417
nicks merged 1 commit into
masterfrom
nicks/vendor

Conversation

@nicks

@nicks nicks commented Apr 9, 2021

Copy link
Copy Markdown
Contributor

Hello @milas,

Please review the following commits I made in branch nicks/vendor:

eee91ba (2021-04-09 15:32:23 -0400)
vendor: latest tilt-apiserver and default to TLS connections

0cdf20b (2021-04-09 15:20:01 -0400)
test: optimize engine tests a bit

Code review reminders, by giving a LGTM you attest that:

  • Commits are adequately tested
  • Code is easy to understand and conforms to style guides
  • Incomplete code is marked with TODOs
  • Code is suitably instrumented with logging and metrics

@nicks
nicks requested a review from milas April 9, 2021 19:32
@nicks

nicks commented Apr 9, 2021

Copy link
Copy Markdown
Contributor Author

this makes the engine tests a bit slower, but i don't totally understand why yet.

i know a lot of the kubebuilder docs recommends a testing convention where you use ginkgo to create a new apiserver at the start of the package and keep it up in between tests, though i'm not sure if there's an easy way to do this without a major test refactor

@nicks

nicks commented Apr 9, 2021

Copy link
Copy Markdown
Contributor Author

ugh, i think i see the problem. generating certs is very expensive and adds a lot of overhead per test. i think i need to refactor this api to generate the certs once

@nicks

nicks commented Apr 12, 2021

Copy link
Copy Markdown
Contributor Author

OK, let's try this again! now with test certs in testing

@nicks
nicks merged commit 52d2b46 into master Apr 12, 2021
@nicks
nicks deleted the nicks/vendor branch April 12, 2021 17:43
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.

2 participants