-
Notifications
You must be signed in to change notification settings - Fork 545
Fix attest extra arguments #3027
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
664d5e7
to
5e9f0ad
Compare
Hum indeed, I forgot the extra arguments support which has been introduced in moby/buildkit#5372. I think we also need docs changes: |
b570e26
to
3db226c
Compare
3db226c
to
89713e4
Compare
Signed-off-by: Laurent Goderre <[email protected]>
89713e4
to
9b2e0c7
Compare
24c1e37
to
630c42a
Compare
writer := csv.NewWriter(&attr) | ||
writer.Write([]string{pair}) | ||
writer.Flush() | ||
pair = strings.TrimSpace(attr.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.
You may add a comment in here that csv writer adds a newline. Not entirely obvious.
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.
Done!
Signed-off-by: Laurent Goderre <[email protected]>
630c42a
to
b06bddf
Compare
The new parsing of attest argument broke support for extra args for the attest command in buildkit as shown with these tests:
https://github.com/moby/buildkit/blob/master/frontend/attestations/parse_test.go#L31-L58
Note that the case should preserved for these args as well as they are set as env var in the scanner.