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

commands/operator-sdk: implement generate k8s #78

Merged
merged 2 commits into from
Mar 1, 2018
Merged

commands/operator-sdk: implement generate k8s #78

merged 2 commits into from
Mar 1, 2018

Conversation

fanminshi
Copy link
Contributor

this pr implements generate k8s command which do a code-generation for the custom resource.

@fanminshi
Copy link
Contributor Author

this depends on code-gen tmpl fix #77 to work completely,.

@fanminshi fanminshi mentioned this pull request Mar 1, 2018
21 tasks
@fanminshi
Copy link
Contributor Author

fanminshi commented Mar 1, 2018

Testing is experiencing some errors:

$ cat tmp/codegen/update-generated.sh
#!/usr/bin/env bash

set -o errexit
set -o nounset
set -o pipefail

DOCKER_REPO_ROOT="/go/src/github.com/coreos/app-operator"
IMAGE=${IMAGE:-"gcr.io/coreos-k8s-scale-testing/codegen"}

docker run --rm \
  -v "$PWD":"$DOCKER_REPO_ROOT" \
  -w "$DOCKER_REPO_ROOT" \
  "$IMAGE" \
  "/go/src/k8s.io/code-generator/generate-groups.sh"  \
  "all" \
  "github.com/coreos/app-operator/pkg/generated" \
  "github.com/coreos/app-operator/pkg/apis" \
  "app:v1alpha1" \
  --go-header-file "./tmp/codegen/boilerplate.go.txt" \
  $@
.
$ operator-sdk generate k8s
Error: failed to perform code-generation for CustomResources: (Generating deepcopy funcs
Generating clientset for app:v1alpha1 at github.com/coreos/app-operator/pkg/generated/clientset
Generating listers for app:v1alpha1 at github.com/coreos/app-operator/pkg/generated/listers
Generating informers for app:v1alpha1 at github.com/coreos/app-operator/pkg/generated/informers
./tmp/codegen/update-generated.sh: line 21: .: filename argument required
.: usage: . filename [arguments]
)

should be fixed by #80

@fanminshi
Copy link
Contributor Author

latest test output:

$ operator-sdk generate k8s
Generating deepcopy funcs
Generating clientset for app:v1alpha1 at github.com/coreos/app-operator/pkg/generated/clientset
Generating listers for app:v1alpha1 at github.com/coreos/app-operator/pkg/generated/listers
Generating informers for app:v1alpha1 at github.com/coreos/app-operator/pkg/generated/informers

@fanminshi
Copy link
Contributor Author

fixed previous error.
PTAL cc/ @hasbro17

}

bcmd := exec.Command(build)
o, err := bcmd.CombinedOutput()
if err != nil {
ExitWithError(ExitError, fmt.Errorf("failed to build: (%v)", string(o)))
cmdError.ExitWithError(cmdError.ExitError, fmt.Errorf("failed to build: (%v)", err))
Copy link
Contributor

Choose a reason for hiding this comment

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

Print stderr:

cmdError.ExitWithError(cmdError.ExitError, fmt.Errorf("failed to build: (%v)", string(o)))

And in all other places that use o, err := bcmd.CombinedOutput()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch.

"os/exec"

cmdError "github.com/coreos/operator-sdk/commands/error"
"github.com/spf13/cobra"
Copy link
Contributor

Choose a reason for hiding this comment

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

Move one line down

@fanminshi
Copy link
Contributor Author

all fixed! PLAL cc/ @hasbro17

@hasbro17
Copy link
Contributor

hasbro17 commented Mar 1, 2018

Is it necessary to have the error utilities commands/error/error.go in the pkg commands/error? I feel like that makes the commands pkg confusing since errors is not really a command.

We should move it to pkg/util/error/error.go.

@fanminshi
Copy link
Contributor Author

I guess this is about scope of the code. I hope that error utilities for commands stays under commands pkg instead of pkg/util/error/error.go..

@fanminshi
Copy link
Contributor Author

take a look of Bad package names under https://blog.golang.org/package-names.

@hasbro17
Copy link
Contributor

hasbro17 commented Mar 1, 2018

I see, you're right. The errors util is only scoped to command/operator-sdk so it should stay there.
But any thoughts on why commands/error and not commands/operator-sdk/error.

@fanminshi
Copy link
Contributor Author

But any thoughts on why commands/error and not commands/operator-sdk/error.

I suspect the error pkg will work with any additional command. e.g operator-xxx. that's why I scoped at commands lvl. However, if it is scoped under commands/operator-sdk/error, that also makes sense. I don't have preference. your call :)

@hasbro17
Copy link
Contributor

hasbro17 commented Mar 1, 2018

I would go for commands/operator-sdk/error for now. And if we add more commands commands/<something-else> that need the errors util we can move it out later to increase the scope.

@fanminshi
Copy link
Contributor Author

sounds good!

@fanminshi
Copy link
Contributor Author

all fixed! PTAL cc/ @hasbro17

@hasbro17
Copy link
Contributor

hasbro17 commented Mar 1, 2018

LGTM

@fanminshi fanminshi merged commit f5ddb69 into operator-framework:master Mar 1, 2018
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.

None yet

2 participants