-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Scorecard2 pod execution #2890
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
Scorecard2 pod execution #2890
Conversation
…age, update scorecard-test to unzip a bundle from the expected mount point
@@ -35,7 +38,7 @@ import ( | |||
// test image. | |||
|
|||
const ( | |||
bundlePath = "/scorecard" | |||
bundleZip = "/scorecard/bundle.zip" |
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.
What is the bundle zip ?
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.
Could we add a comment explaining ?
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.
good catch, originally I was using zip and then switched to tar to archive up the bundle contents, I've updated this and added a comment.
Co-Authored-By: Joe Lanford <[email protected]>
Co-Authored-By: Joe Lanford <[email protected]>
@@ -30,13 +31,13 @@ func getTestResults(client kubernetes.Interface, tests []ScorecardTest) (output | |||
p := tests[i].TestPod | |||
logBytes, err := getPodLog(client, p) | |||
if err != nil { | |||
fmt.Printf("error getting pod log %s\n", err.Error()) | |||
log.Errorf("Error getting pod log %s\n", err.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.
Rather than logging an error, we should convert this error to a ScorecardTestResult
and append it to the list of results. Ditto for any other errors related to anything happening in a test run.
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.
fixed.
@@ -83,12 +99,15 @@ func selectTests(selector labels.Selector, tests []ScorecardTest) []ScorecardTes | |||
|
|||
// runTest executes a single test | |||
// TODO once tests exists, handle the test output | |||
func runTest(test ScorecardTest) error { | |||
func runTest(o Options, test ScorecardTest) (result *v1.Pod, err 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.
This implementation is going to make it difficult to write unit tests for the RunTests
function. WDYT about making a TestRunner
interface to use instead.
type TestRunner
func RunTest(context.Context, test Test, bundle *registry.Bundle) (*TestResult, error)
}
And then have the main struct (Options
right now, maybe Scorecard
based on another comment), have something like:
type Scorecard struct {
Config Config
Selector labels.Selector
WaitTime time.Duration
Bundle *registry.Bundle
TestRunner TestRunner
}
var _ TestRunner = &PodTestBundleRunner{}
type PodTestRunner struct {
Client kubernetes.Interface
Namespace string
ServiceAccount string
SkipCleanup bool
}
func (r *PodTestRunner) RunTest(ctx context.Context, test Test, bundle *registry.Bundle) (*TestResult, error) {
return actuallyRunTheTest(ctx, test, bundle)
}
var _ TestRunner = &FakeTestRunner{}
type FakeTestRunner struct {
TestResult *TestResult
Error error
}
func (r *FakeTestRunner) RunTest(ctx context.Context, test Test, bundle *registry.Bundle) (*TestResult, error) {
return r.TestResult, r.Error
}
Then on the cmd CLI side, we can construct the TestRunner and set that field on the scorecard struct.
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.
interesting approach, but I'm wondering if we could address this in another PR since I don't think it impacts the test interface and I'd like to unblock the folks writing tests currently. I think there is a story to rewrite the scorecard tests, maybe we could do this in that story?
Co-Authored-By: Joe Lanford <[email protected]>
Co-Authored-By: Joe Lanford <[email protected]>
Co-Authored-By: Joe Lanford <[email protected]>
Co-Authored-By: Joe Lanford <[email protected]>
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.
LGTM
Description of the change:
this PR adds functionality to the alpha scorecard implementation, you can test
this PR out by running the following command as an example:
this PR includes the following:
Motivation for the change:
this is part of a larger set of work to implement the alpha scorecard, but this PR is mostly focused on wiring up the scorecard command to have test pods created, and test pod log transformed into scorecard output format for user reporting.