-
Notifications
You must be signed in to change notification settings - Fork 103
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
Implement CLI and E2E testing using the KUDO test harness. #656
Implement CLI and E2E testing using the KUDO test harness. #656
Conversation
} | ||
|
||
// Kubeconfig converts a rest.Config into a YAML kubeconfig and writes it to w | ||
func Kubeconfig(cfg *rest.Config, w io.Writer) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't able to find a method that could convert a rest.Config to a kubeconfig already. There's a number of cases that we need to be able to support:
- Merged kubeconfig
- In cluster config
- Mocked control plane (just requires setting the Server setting)
- Kind config
In some cases we have the path and could copy it.. but other times the path might not exist or be ambiguous and we would want to generate a kubeconfig.
But maybe there's a better way to do this.. Thoughts?
a4953d6
to
445d3d1
Compare
@alenkacz that create-operator-in-operator test started failing on here pretty frequently and I noticed that when the test passes it's always close to the timeout (28 seconds or so). I think the tests I've added here have pushed it over the edge to where it times out more often than not.. But I think I'm on to something. When I limit the test parallelism to 2 tests at once instead of 4, everything is a lot faster. This leads me to wonder if either:
When running the tests, I noticed the PlanExecution controller trying to fetch lots of instances even once they were deleted, I wonder if it's doing a lot of potentially unnecessary work:
|
fc6730b
to
fbf1ac0
Compare
Yup, that did it. That last commit that makes
That's down from the 76 seconds successful runs took before:
I'm going to drop that commit from here and put it into a separate PR. |
fbf1ac0
to
445d3d1
Compare
Okay, submitted that fix as #657 :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have couple of questions but overall it looks good :)
Makefile
Outdated
@@ -117,6 +117,11 @@ generate: | |||
generate-clean: | |||
rm -rf hack/code-gen | |||
|
|||
.PHONY: cli-test | |||
# Build CLI for tests | |||
cli-test: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we need a special build for tests? why is make cli
not good enough?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make cli
also runs all of the code generation and such so it takes quite a while to run (30 seconds or so?). As well as all of the linting (personally, I like to lint before I commit but that's just me). I thought it made sense to have a separate build that is quick for testing. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually agree with you now that I think about it, everytime I am building CLI locally I hate when it stops on linting error. WDYT then about having just one build and one that is fast? If not we should at least comment why we need two.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So it actually occurs to me that we don't use make cli
in CI or releasing at all - if it is used, it's used only by developers while developing.
Maybe we just drop the slow one then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm gonna take your suggestion:
make cli
make cli-fast
And then we can discuss what to do about the other targets separately.
Fixes kudobuilder#368 (support CLI commands) Fixes kudobuilder#369 (support install kudo frameworks) Fixes kudobuilder#520 (panic when executing test harness) This adds a `kubectl` setting to the KUDO test harness TestSuite and TestStep configurations to support running kubectl commands as a part of tests.
445d3d1
to
e58ba10
Compare
Okay, I think I addressed everything! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have only one comment in the makefile, otherwise nice work 👏
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alenkacz, jbarrick-mesosphere The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
I've set the do not merge label to make sure you have time to see my makefile comment even though it's not a huge issue :) |
/hold cancel |
What type of PR is this?
/kind feature
What this PR does / why we need it:
This adds a
kubectl
setting to the KUDO test harness TestSuite and TestStep configurations to support running kubectl commands as a part of tests.Which issue(s) this PR fixes:
Fixes #368 (support CLI commands)
Fixes #369 (support install kudo frameworks)
Fixes #520 (panic when executing test harness)
Special notes for your reviewer:
Does this PR introduce a user-facing change?: