-
Notifications
You must be signed in to change notification settings - Fork 119
Extend support for testing from locally built artifacts #95
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
Conversation
kubetest2-gce/deployer/build.go
Outdated
@@ -22,6 +22,7 @@ import ( | |||
"strings" | |||
|
|||
"k8s.io/klog" | |||
"sigs.k8s.io/kubetest2/pkg/build" |
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.
nit: group with import below (this is a first party import)
@@ -23,6 +23,7 @@ import ( | |||
"strings" | |||
|
|||
"k8s.io/klog" | |||
"sigs.k8s.io/kubetest2/pkg/build" |
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.
same here with import grouping, first party imports go in their own group
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.
this also applies to other files
pkg/testers/ginkgo/ginkgo.go
Outdated
return t.Test() | ||
} | ||
|
||
// initializes relevant information from the well defined kubetest2 environment variables. | ||
func (t *Tester) initKubetest2Info() { | ||
t.runDir = os.Getenv("KUBETEST2_RUN_DIR") |
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.
no defaulting?
if err != nil { | ||
return err | ||
} | ||
// make sure we close the file |
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 is this error reported but not in.Close() ?
pkg/app/app.go
Outdated
@@ -124,6 +124,9 @@ func RealMain(opts types.Options, d types.Deployer, tester types.Tester) (result | |||
|
|||
envsForTester := os.Environ() | |||
// We expose both ARIFACTS and KUBETEST2_RUN_DIR so we can more granular about caching vs output in future. | |||
// also add run_dir to $PATH for locally built binaries | |||
dollarPath := opts.RunDir() + string(filepath.ListSeparator) + os.Getenv("PATH") |
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.
nit: updatedPath
, $
is specific to shell.
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.
/approve
/lgtm
I have one nit but it can be done as followup
var ( | ||
CommonTestBinaries = []string{ |
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.
Any reason this needs to be a var
?
var ( | |
CommonTestBinaries = []string{ | |
const ( | |
CommonTestBinaries = []string{ |
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.
go doesn't support array constants?
https://golang.org/ref/spec#Constants
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: amwat, spiffxp 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 |
Add functionality to the ginkgo tester to be able to test from current builds.
somewhat makes it easier for #87
This will help solve skew problems e.g. kubectl https://testgrid.k8s.io/conformance-all#Conformance%20-%20GCE%20-%20master%20-%20kubetest2
Also refactor kind deployer a bit.
local testing is now as easy as :)
xref: kubernetes/enhancements#2464
/cc @BenTheElder @spiffxp