-
Notifications
You must be signed in to change notification settings - Fork 286
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
pack build allows multiple --env-file
#291
Conversation
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.
Thanks for the contribution!
LGTM for the most part. Minor format change necessary.
commands/build_test.go
Outdated
"github.com/golang/mock/gomock" | ||
"github.com/sclevine/spec" | ||
"github.com/sclevine/spec/report" | ||
"github.com/spf13/cobra" |
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.
Our format validation is failing on CI because these need to be broken up into 3 groups in this order:
- standard library
- 3rd party
- current project
If you are on MacOS or Linux you should be able to run make format
.
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.
Thanks a lot! I've pushed a fix
Fix buildpacks#264 Signed-off-by: David Gageot <david@gageot.net>
Signed-off-by: David Gageot <david@gageot.net>
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.
Looks great! Thanks for the contribution and for improving our test coverage. I am requesting two minor changes to test cleanup and test descriptions.
envPath = envfile.Name() | ||
}) | ||
|
||
it("builds an image with a builder", func() { |
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.
It would be better if the description strings in each it
described what this particular test was asserting on (i.e. env vars in this case)
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.
It would be great to have your feature in the release we intend to cut today. Therefore I am going to approve and merge this and we can fix up the nitpicks from from my review.
thanks again!
Thanks. I'm on it |
Those are fixes to buildpacks#291: + Delete temp files after the test + Give better description for test cases Signed-off-by: David Gageot <david@gageot.net>
Fixes #264