Included tests for GitHub attestations #61
Conversation
naveensrinivasan
commented
Oct 26, 2023
naveensrinivasan
commented
- Included tests for GitHub attestations and some simple clean up.
naveensrinivasan
force-pushed
the
naveen/attest/github
branch
from
f69933d
to
d40829d
Compare
naveensrinivasan
force-pushed
the
naveen/attest/github
branch
2 times, most recently
from
9016661
to
ed815b8
Compare
- Included tests for GitHub attestations and some simple clean up. Signed-off-by: naveensrinivasan <[email protected]>
Signed-off-by: naveensrinivasan <[email protected]>
jkjell
force-pushed
the
naveen/attest/github
branch
from
ed815b8
to
03a833d
Compare
| assert.NotNil(t, subjects) | ||
| assert.Equal(t, 2, len(subjects)) | ||
|
|
||
| expectedSubjects := []string{"pipelineurl:" + attestor.PipelineUrl, "projecturl:" + attestor.ProjectUrl} |
There was a problem hiding this comment.
maybe there is something that I am overlooking here, but will attestor.PipelineUrl and attestor.ProjectUrl both return empty strings? of course that would technically be fine, but I wanted to ensure that this would be the case. Maybe we could populate these fields in the attestor instantiation above?
Signed-off-by: Tom Meadows <[email protected]>
|
After reviewing these tests look fine to me, and once again seem worthy additions to the codebase. Happy to approve once my question has been answered 😄 |
| attestor := &Attestor{ | ||
| aud: "projecturl", | ||
| jwksURL: tokenServer.URL, | ||
| tokenURL: os.Getenv("ACTIONS_ID_TOKEN_REQUEST_URL"), |
There was a problem hiding this comment.
| tokenURL: os.Getenv("ACTIONS_ID_TOKEN_REQUEST_URL"), | |
| tokenURL: os.Getenv("ACTIONS_ID_TOKEN_REQUEST_URL"), | |
| PipelineUrl: "https://github.com/in-toto/go-witness/actions/runs/234872349827", | |
| ProjectUrl: "https://github.com/in-toto/go-witness", |
There was a problem hiding this comment.
Something like this aligns to the suggestion in comment below. Not sure if this is unnecessary / not optimal.
|
Tests seem to be running fine. Unless others (@kairoaraujo, @jkjell, @mikhailswift) have feedback, I am happy to merge approve and merge. Just the last comment and suggestion outstanding but I don't think it's a big deal. |
|
I'm going to go ahead and merge this - pretty happy with what it's doing. |
