Skip to content

commands/{build,cmdutil},pkg/generator: remove build.sh#561

Merged
AlexNPavel merged 4 commits into
operator-framework:masterfrom
AlexNPavel:build-sh-2
Oct 2, 2018
Merged

commands/{build,cmdutil},pkg/generator: remove build.sh#561
AlexNPavel merged 4 commits into
operator-framework:masterfrom
AlexNPavel:build-sh-2

Conversation

@AlexNPavel
Copy link
Copy Markdown
Contributor

This commit moves all functionality of build.sh into the build.go
file to embed the functionality into the binary. It also allows the
binary to generate the files needed when making tests (Dockerfile
and go-test.sh) in case someone tries to build testing enabled
images using a project made before that functionality was added.

See #541

This commit moves all functionality of build.sh into the build.go
file to embed the functionality into the binary. It also allows the
binary to generate the files needed when making tests (Dockerfile
and go-test.sh) in case someone tries to build testing enabled
images using a project made before that functionality was added.

See operator-framework#541
@openshift-ci-robot openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 2, 2018
@AlexNPavel AlexNPavel requested a review from hasbro17 October 2, 2018 21:09
@openshift-ci-robot openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 2, 2018
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
_, err = os.Stat("tmp/build/test-framework/Dockerfile")
if err != nil {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

maybe add a logging to indicate that you are missing the tmp/build/test-framework/Dockerfile and you are going to tmp/build/test-framework/Dockerfile

Copy link
Copy Markdown
Contributor

@fanminshi fanminshi Oct 2, 2018

Choose a reason for hiding this comment

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

offline discussion: generator already prints out the file it is going to generate. so we don't need to add extra a logging statement.

@fanminshi
Copy link
Copy Markdown
Contributor

lgtm

Comment thread commands/operator-sdk/cmd/build.go Outdated
buildTestCmd.Env = goBuildEnv
o, err := buildTestCmd.CombinedOutput()
if err != nil {
cmdError.ExitWithError(cmdError.ExitError, fmt.Errorf("failed to build: (%v)", string(o)))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Change this to:

log.Fatalf("failed to build: %v (%v)", err, string(o)))

And on Line 161 as well. We're phasing out cmdError.ExitWithError since that doesn't really add much.

@hasbro17
Copy link
Copy Markdown
Contributor

hasbro17 commented Oct 2, 2018

LGTM after nit

@AlexNPavel AlexNPavel merged commit a7d30d0 into operator-framework:master Oct 2, 2018
@AlexNPavel AlexNPavel deleted the build-sh-2 branch October 2, 2018 23:20
cmdError.ExitWithError(cmdError.ExitBadArgs, fmt.Errorf("build command needs exactly 1 argument"))
}

cmdutil.MustInProjectRoot()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This command breaks support for Ansible Operator because config/config.yaml doesn't exist in our project. Why doesn't this function look for the Dockerfile instead? It would verify we are in the project root and be agnostic to the type of operator.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants