Skip to content

commands/.../build.go: set exec command output to stdout, stderr #692

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

Merged
merged 1 commit into from
Nov 5, 2018
Merged

commands/.../build.go: set exec command output to stdout, stderr #692

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 18 additions & 14 deletions commands/operator-sdk/cmd/build.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,17 +144,18 @@ func buildFunc(cmd *cobra.Command, args []string) {
log.Fatalf("could not identify current working directory: %v", err)
}

// Don't need to buld go code if Ansible Operator
// Don't need to build go code if Ansible Operator
if mainExists() {
managerDir := filepath.Join(projutil.CheckAndGetCurrPkg(), scaffold.ManagerDir)
outputBinName := filepath.Join(wd, scaffold.BuildBinDir, filepath.Base(wd))
buildCmd := exec.Command("go", "build", "-o", outputBinName, managerDir)
buildCmd.Env = goBuildEnv
o, err := buildCmd.CombinedOutput()
buildCmd.Stdout = os.Stdout
buildCmd.Stderr = os.Stderr
err = buildCmd.Run()
if err != nil {
log.Fatalf("failed to build operator binary: %v (%v)", err, string(o))
log.Fatalf("failed to build operator binary: %v", err)
}
fmt.Fprintln(os.Stdout, string(o))
}

image := args[0]
Expand All @@ -163,25 +164,27 @@ func buildFunc(cmd *cobra.Command, args []string) {
baseImageName += "-intermediate"
}
dbcmd := exec.Command("docker", "build", ".", "-f", "build/Dockerfile", "-t", baseImageName)
o, err := dbcmd.CombinedOutput()
dbcmd.Stdout = os.Stdout
dbcmd.Stderr = os.Stderr
err = dbcmd.Run()
if err != nil {
if enableTests {
log.Fatalf("failed to build intermediate image for %s image: %v (%s)", image, err, string(o))
log.Fatalf("failed to build intermediate image for %s image: %v", image, err)
} else {
log.Fatalf("failed to output build image %s: %v (%s)", image, err, string(o))
log.Fatalf("failed to output build image %s: %v", image, err)
}
}
fmt.Fprintln(os.Stdout, string(o))

if enableTests {
testBinary := filepath.Join(wd, scaffold.BuildBinDir, filepath.Base(wd)+"-test")
buildTestCmd := exec.Command("go", "test", "-c", "-o", testBinary, testLocationBuild+"/...")
buildTestCmd.Env = goBuildEnv
o, err := buildTestCmd.CombinedOutput()
buildTestCmd.Stdout = os.Stdout
Copy link
Member

Choose a reason for hiding this comment

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

If we are going to print building the intermediate image, I don't see us log to the user that is what is happening. I think it might be nice to do that so when they see two docker builds happening they are not confused. Thoughts?

Copy link
Member Author

@joelanford joelanford Nov 2, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

We don't log anything right now unless there's a failure. If we add a log for one of the steps, do you think it makes sense to go ahead and log all of the steps?

Copy link
Member Author

Choose a reason for hiding this comment

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

^^ Thoughts?

We're currently printing the intermediate image build output without a log saying what's happening, so this PR doesn't change that.

I agree that adding some logging for context would be helpful though. Would logging before every subprocess be unnecessarily verbose?

Without --enable-tests, the only output is the docker build output for the final image, so users should understand that with no additional context. With --enable-tests, the output is the docker build output for both the intermediate and final image. So maybe we only log both docker builds, but only when tests are enabled?

Copy link
Member

@estroz estroz Nov 5, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I agree that there should be a log.Print per step. We can add that in a separate PR; I'm actually working on rewriting our log usage so I can add statements for each step.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good to me. In that case, I'll merge this as is.

buildTestCmd.Stderr = os.Stderr
err = buildTestCmd.Run()
if err != nil {
log.Fatalf("failed to build test binary: %v (%v)", err, string(o))
log.Fatalf("failed to build test binary: %v", err)
}
fmt.Fprintln(os.Stdout, string(o))
// if a user is using an older sdk repo as their library, make sure they have required build files
testDockerfile := filepath.Join(scaffold.BuildTestDir, scaffold.DockerfileFile)
_, err = os.Stat(testDockerfile)
Expand All @@ -206,11 +209,12 @@ func buildFunc(cmd *cobra.Command, args []string) {
}

testDbcmd := exec.Command("docker", "build", ".", "-f", testDockerfile, "-t", image, "--build-arg", "NAMESPACEDMAN="+namespacedManBuild, "--build-arg", "BASEIMAGE="+baseImageName)
o, err = testDbcmd.CombinedOutput()
testDbcmd.Stdout = os.Stdout
testDbcmd.Stderr = os.Stderr
err = testDbcmd.Run()
if err != nil {
log.Fatalf("failed to output build image %s: %v (%s)", image, err, string(o))
log.Fatalf("failed to output build image %s: %v", image, err)
}
fmt.Fprintln(os.Stdout, string(o))
// Check image name of deployments in namespaced manifest
verifyTestManifest(image)
}
Expand Down