Skip to content

feat(runner): allow for optional environment variables #443

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 12 commits into from
Nov 30, 2023
Merged

feat(runner): allow for optional environment variables #443

merged 12 commits into from
Nov 30, 2023

Conversation

buccarel
Copy link
Contributor

@buccarel buccarel commented Nov 16, 2023 •
edited
Loading

This PR fixes the following issue:
#440

@buccarel
feat(runner): allow for optional environment variables
3cebefb 
Signed-off-by: Mattia Buccarella <m.buccarella@gmail.com>
buccarel
buccarel commented Nov 16, 2023
View reviewed changes
internal/attestation/crafter/runners/runner.go
@@ -0,0 +1,43 @@
//
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this file would be better named "common" or something similar

@buccarel
fix params
7b1c245 
Signed-off-by: Mattia Buccarella <m.buccarella@gmail.com>
@buccarel
finishing up
c16973e 
Signed-off-by: Mattia Buccarella <m.buccarella@gmail.com>
migmartri
migmartri reviewed Nov 21, 2023
View reviewed changes
Copy link
Member

@migmartri migmartri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good, ptal at some comments, especially around not enforcing runner check on development

Thanks!

@buccarel
addressed some feedback
29ae09b 
Signed-off-by: Mattia Buccarella <m.buccarella@gmail.com>
@buccarel
wrapping error
840423e 
Signed-off-by: Mattia Buccarella <m.buccarella@gmail.com>
@buccarel buccarel self-assigned this Nov 28, 2023
@buccarel
test without error when optional vars are missing
7d45dd2 
Signed-off-by: Mattia Buccarella <m.buccarella@gmail.com>
@buccarel
adjusting test
7f2b0da 
Signed-off-by: Mattia Buccarella <m.buccarella@gmail.com>
@buccarel
performance/lint
ae20a62 
Signed-off-by: Mattia Buccarella <m.buccarella@gmail.com>
@buccarel buccarel marked this pull request as ready for review November 28, 2023 12:33
migmartri
migmartri reviewed Nov 28, 2023
View reviewed changes
Copy link
Member

@migmartri migmartri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM mostly!

Ptal mainly at my comment about the behavior on the status command.

Thanks!

@buccarel
ResolveEnvVars can return multiple errors
433aba4 
Signed-off-by: Mattia Buccarella <m.buccarella@gmail.com>
@buccarel buccarel marked this pull request as draft November 30, 2023 14:04
@buccarel
adjusting test
6f68cce 
Signed-off-by: Mattia Buccarella <m.buccarella@gmail.com>
@buccarel
adjusting tests and add note
26b9ca2 
Signed-off-by: Mattia Buccarella <m.buccarella@gmail.com>
@buccarel
adjusting expected error message in a test
152d850 
Signed-off-by: Mattia Buccarella <m.buccarella@gmail.com>
@buccarel buccarel marked this pull request as ready for review November 30, 2023 15:01
migmartri
migmartri approved these changes Nov 30, 2023
View reviewed changes
Copy link
Member

@migmartri migmartri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@buccarel buccarel merged commit 4a6e458 into chainloop-dev:main Nov 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@migmartri migmartri migmartri approved these changes

Assignees

@buccarel buccarel

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@buccarel @migmartri