fix: add nil/len guards for RunCommand #126
Conversation
|
This PR is missing more tests for the InTotoRun function. Right now I am only testing the RunCommand func. We might want to test link creation for an empty CmdArgs parameter as well |
| @@ -360,6 +360,23 @@ func TestRunCommand(t *testing.T) { | |||
| } | |||
| } | |||
|
|
|||
| func TestRunCommandErrors(t *testing.T) { | |||
There was a problem hiding this comment.
Can / should we also test by invoking InTotoRun?
There was a problem hiding this comment.
Oh whoops, I missed your comment on the PR.
There was a problem hiding this comment.
yes we can. I will add a few more tests for this.
There was a problem hiding this comment.
@adityasaky I had a closer look on this. We cannot test InTotoRun for it, because we do not fail if the cmdArgs are empty. If we would fail if cmdArgs is empty we would no longer support link files without commands:
// make sure that we only run RunCommand if cmdArgs is not nil or empty
var byProducts map[string]interface{}
if cmdArgs != nil && len(cmdArgs) != 0 {
byProducts, err = RunCommand(cmdArgs, runDir)
if err != nil {
return linkMb, err
}
}See line 3 that checks for empty cmdArgs
There was a problem hiding this comment.
Reviving this one. I see what you mean but I think it's okay to test that the no command flag works as intended and still results in a link, no?
There was a problem hiding this comment.
@shibumi can you take a look? I added a test checking if link metadata is successfully generated. Also caught that a minor difference in byProducts had crept in.
shibumi
force-pushed
the
shibumi/fix-124
branch
from
d7be895 to
37e9e08
Compare
shibumi
force-pushed
the
shibumi/fix-124
branch
2 times, most recently
from
53237ad to
aa5af7e
Compare
|
We should add |
Is it even possible to create an in-toto link attestation without a command being run? |
|
Yes, the field should just be empty. The reference implementation supports it too. Also note that links generates via record always have the command field empty. |
|
Just to be sure:
The |
shibumi
force-pushed
the
shibumi/fix-124
branch
from
aa5af7e to
5bb106f
Compare
Yes. |
Signed-off-by: Aditya Sirish <aditya@saky.in>
adityasaky
force-pushed
the
shibumi/fix-124
branch
from
8fbc953 to
9d1c911
Compare
Signed-off-by: Aditya Sirish <aditya@saky.in>
adityasaky
force-pushed
the
shibumi/fix-124
branch
from
9d1c911 to
53bea4f
Compare
Please fill in the fields below to submit a pull request. The more
information that is provided, the better.
Fixes issue #: #124
Description of pull request:
Please verify and check that the pull request fulfills the following
requirements: