-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
add custom parameters to structured tests #6055
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
test: | ||
- image: gcr.io/k8s-skaffold/skaffold-example | ||
structureTests: | ||
- './structure-test/*' | ||
structureTestsArgs: | ||
- --driver tar | ||
- -q | ||
- --no-color | ||
- --test-report TEST_REPORT_NAME |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,6 +22,7 @@ import ( | |
"io" | ||
"os" | ||
"os/exec" | ||
"strings" | ||
|
||
"github.com/sirupsen/logrus" | ||
|
||
|
@@ -32,11 +33,12 @@ import ( | |
) | ||
|
||
type Runner struct { | ||
structureTests []string | ||
imageName string | ||
imageIsLocal bool | ||
workspace string | ||
localDaemon docker.LocalDaemon | ||
structureTests []string | ||
imageName string | ||
imageIsLocal bool | ||
workspace string | ||
localDaemon docker.LocalDaemon | ||
structureTestArgs []string | ||
} | ||
|
||
// New creates a new structure.Runner. | ||
|
@@ -46,11 +48,12 @@ func New(cfg docker.Config, tc *latestV1.TestCase, imageIsLocal bool) (*Runner, | |
return nil, err | ||
} | ||
return &Runner{ | ||
structureTests: tc.StructureTests, | ||
imageName: tc.ImageName, | ||
workspace: tc.Workspace, | ||
localDaemon: localDaemon, | ||
imageIsLocal: imageIsLocal, | ||
structureTests: tc.StructureTests, | ||
imageName: tc.ImageName, | ||
workspace: tc.Workspace, | ||
localDaemon: localDaemon, | ||
imageIsLocal: imageIsLocal, | ||
structureTestArgs: tc.StructureTestArgs, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it might be better to move this up below line 51 with the |
||
}, nil | ||
} | ||
|
||
|
@@ -86,7 +89,10 @@ func (cst *Runner) runStructureTests(ctx context.Context, out io.Writer, imageTa | |
for _, f := range files { | ||
args = append(args, "--config", f) | ||
} | ||
|
||
for _, customArg := range cst.structureTestArgs { | ||
splittedCustomArg := strings.Fields(customArg) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We require the user to be responsible for splitting arguments properly. Otherwise we need to deal with quoting issues, and YAML already makes that hard enough. Docker uses this approach too. |
||
args = append(args, splittedCustomArg...) | ||
} | ||
cmd := exec.CommandContext(ctx, "container-structure-test", args...) | ||
cmd.Stdout = out | ||
cmd.Stderr = out | ||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -94,6 +94,44 @@ func TestIgnoreDockerNotFound(t *testing.T) { | |||||||||||||||
}) | ||||||||||||||||
} | ||||||||||||||||
|
||||||||||||||||
func TestCustomParams(t *testing.T) { | ||||||||||||||||
const ( | ||||||||||||||||
imageName = "image:tag" | ||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just inline this: you're using the string inline anyways further below. |
||||||||||||||||
) | ||||||||||||||||
|
||||||||||||||||
testutil.Run(t, "", func(t *testutil.T) { | ||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would be useful to have tests here for: |
||||||||||||||||
tmpDir := t.NewTempDir().Touch("test.yaml") | ||||||||||||||||
t.Override(&cluster.FindMinikubeBinary, func() (string, semver.Version, error) { return "", semver.Version{}, errors.New("not found") }) | ||||||||||||||||
|
||||||||||||||||
expectedBaseCmd := "container-structure-test test -v warn --image " + imageName + " --config " + tmpDir.Path("test.yaml") | ||||||||||||||||
t.Override(&util.DefaultExecCommand, testutil.CmdRun(expectedBaseCmd+" --driver tar --force --no-color -q --save")) | ||||||||||||||||
|
||||||||||||||||
structureTestArgs := []string{"--driver tar", "--force", "--no-color", "-q", "--save"} | ||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||
|
||||||||||||||||
cfg := &mockConfig{ | ||||||||||||||||
tests: []*latestV1.TestCase{{ | ||||||||||||||||
ImageName: "image", | ||||||||||||||||
Workspace: tmpDir.Root(), | ||||||||||||||||
StructureTests: []string{"test.yaml"}, | ||||||||||||||||
StructureTestArgs: structureTestArgs, | ||||||||||||||||
}}, | ||||||||||||||||
} | ||||||||||||||||
|
||||||||||||||||
testCase := &latestV1.TestCase{ | ||||||||||||||||
ImageName: "image", | ||||||||||||||||
Workspace: tmpDir.Root(), | ||||||||||||||||
StructureTests: []string{"test.yaml"}, | ||||||||||||||||
StructureTestArgs: structureTestArgs, | ||||||||||||||||
} | ||||||||||||||||
testEvent.InitializeState([]latestV1.Pipeline{{}}) | ||||||||||||||||
|
||||||||||||||||
testRunner, err := New(cfg, testCase, true) | ||||||||||||||||
t.CheckNoError(err) | ||||||||||||||||
err = testRunner.Test(context.Background(), ioutil.Discard, "image:tag") | ||||||||||||||||
t.CheckNoError(err) | ||||||||||||||||
}) | ||||||||||||||||
} | ||||||||||||||||
|
||||||||||||||||
type mockConfig struct { | ||||||||||||||||
runcontext.RunContext // Embedded to provide the default values. | ||||||||||||||||
tests []*latestV1.TestCase | ||||||||||||||||
|
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.
As I mention elsewhere in this review, we don't want to do arg parsing. But most long-form arguments support using an
=
so we should be able to use: