Skip to content
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

Merged
merged 8 commits into from
Jun 26, 2020
Merged

Build config #8

merged 8 commits into from
Jun 26, 2020

Conversation

EnriqueL8
Copy link
Owner

What type of PR is this?

/kind feature

What does does this PR do / why we need it:
Add BuildConfig path.

err := makeTar(localPath, dest, copyFiles, globExps, tarWriter)
if err != nil {
log.Errorf("Error while creating tar: %#v", err)
os.Exit(1)
Copy link

@upLukeWeston upLukeWeston Jun 26, 2020

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).

Copy link
Owner Author

Choose a reason for hiding this comment

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

Same as copy file

@@ -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
Copy link

@upLukeWeston upLukeWeston Jun 26, 2020

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!

Copy link
Owner Author

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

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?

Copy link

@upLukeWeston upLukeWeston left a comment

Choose a reason for hiding this comment

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

LGTM.

@EnriqueL8 EnriqueL8 merged commit 1e2c221 into deploy_command Jun 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants