-
Notifications
You must be signed in to change notification settings - Fork 0
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
Build config #8
Build config #8
Conversation
err := makeTar(localPath, dest, copyFiles, globExps, tarWriter) | ||
if err != nil { | ||
log.Errorf("Error while creating tar: %#v", err) | ||
os.Exit(1) |
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.
Did you mean to keep the log & os.Exit here, or should it just be changed to a return errors.Wrap(err, "error while creating tar")
This is also the case in GetTarReader
but I can't leave a comment on that bit because it's not changed (the log & os.Exit must have originally been my bad).
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.
Same as copy file
pkg/sync/adapter.go
Outdated
@@ -31,8 +32,8 @@ type Adapter struct { | |||
common.AdapterContext | |||
} | |||
|
|||
// SyncFilesBuild sync the local files to build container volume | |||
func (a Adapter) SyncFilesBuild(buildParameters common.BuildParameters, compInfo common.ComponentInfo) (syncFolder string, err error) { | |||
// SyncsFilesBuild sync the local files to build container volume |
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.
Should be SyncFilesBuild
, this'll throw a warning because of your additional 's'. I don't mean to be picky, but you did say be harsh!
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.
I thought I change that :(, will update
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.
I had changed it on my branch that you'd merged, so maybe it changed back during a merge conflict or something?
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.
LGTM.
What type of PR is this?
What does does this PR do / why we need it:
Add BuildConfig path.