*: operator-sdk new command generates project skeleton by fanminshi · Pull Request #21 · operator-framework/operator-sdk

fanminshi · 2018-02-19T01:59:51Z

operator-sdk new command now generates the project skeleton based on the new input.

operator-sdk new play-operator creates the follow defaults dirs:

├── cmd
│   └── play-operator
├── config
├── deploy
│   └── play-operator
├── pkg
│   ├── apis
│   │   └── play
│   │       └── v1
│   └── stub
└── tmp
    ├── build
    └── codegen

fanminshi · 2018-02-19T02:00:01Z

cc/ @hasbro17 @hongchaodeng

hongchaodeng · 2018-02-19T08:20:42Z

Please do not generate pkg/apis by default.
See #20 (comment).
The default PlayService isn't very reusable. We should not have it by default.

fanminshi · 2018-02-19T22:23:16Z

let's finalize the skd command's interface and its behavior before continuing this pr; coding is simple but thinking is hard :)

hongchaodeng · 2018-02-20T21:37:56Z

+	codegenDir      = tmpDir + "/codegen"
+	pkgDir          = "pkg"
+	apisDir         = pkgDir + "/apis"
+	clientDir       = pkgDir + "/client"


No need for client as discussed.

less is more :)

hongchaodeng · 2018-02-20T21:38:50Z

-	err = os.MkdirAll(filepath.Join(projDir, "pkg/apis", groupName, apiVersion), defaultFileMode)
-	if err != nil {
+func (g *Generator) renderDeploy() error {
+	if err := os.MkdirAll(filepath.Join(deployDir, g.projectName), defaultFileMode); err != nil {


Only deploy/.
There will be a file <project-name>.yaml, e.g. memcached-operator.yaml.

fine with me!

hongchaodeng · 2018-02-20T21:39:53Z

@fanminshi
When you address the change, can you also paste the layout after changed? Thanks!

fanminshi · 2018-02-20T21:49:40Z

@hongchaodeng yeah, i'll do that.

fanminshi · 2018-02-20T22:22:42Z

test:

func TestGenDirs(t *testing.T) {
	gen := NewGenerator("app.example.com/v1alpha1", "AppService", "app-operator")
	if err := gen.Render(); err != nil {
		t.Error(err)
	}
}

output:

├── app-operator
│   ├── cmd
│   │   └── app-operator
│   ├── config
│   ├── deploy
│   ├── pkg
│   │   ├── apis
│   │   │   └── app
│   │   │       └── v1alpha1
│   │   └── stub
│   └── tmp
│       ├── build
│       └── codegen

cc/ @hongchaodeng @hasbro17

hongchaodeng · 2018-02-20T22:26:28Z

 }

+// Render generates the default project structure.
 func (g *Generator) Render() error {


Add the ascii graph here?
This would be useful since we would keep it in sync when changing the layout.
Ideally we should also have test.

kk, let me think of something.

fanminshi · 2018-02-20T22:33:52Z

all fixed. PTAL cc/ @hongchaodeng @hasbro17

hasbro17 · 2018-02-20T22:48:00Z

 import (
 	"fmt"

+	"github.com/coreos/operator-sdk/pkg/generator"


Leave space between non operator-sdk imports.

hongchaodeng · 2018-02-20T22:49:52Z

LGTM

hasbro17 · 2018-02-20T22:55:40Z

+// it uses the first word separated with "." of the groupName as the api directory name.
+// for example,
+//  apiDirName("app.example.com/v1alpha1") => "app".
+func apiDirName(apiGroup string) string {


We should do some error checking here on the format of the string, like if the user enters app/v1alpha.
Same for groupName() and apiVersion() functions.

@hasbro17 I have a todo in https://github.com/coreos/operator-sdk/blob/master/commands/operator-sdk/cmd/new.go#L65 for that.

I think it is better to check input on top lvl so that the lower lvl code doesn't have to worry about it.

@fanminshi Can you follow this comment in a new PR?

@hongchaodeng it is a TODO inside cmd/new.go.

I added this as part of the Action Items.

hasbro17 · 2018-02-20T23:11:39Z

 }
+
+// apiVersion extracts api version from the given apiGroup.
+func apiVersion(apiGroup string) string {


So the APIVersion string is made up of group/version, e.g the APIVersion string app.example.com/v1alpha1 has group app.example.com and version v1alpha1.

So probably modify this func signature to

// version extracts version from the given apiVersion func version(apiVersion string) string {

Similarly use the name apiVersion instead of apiGroup as the argument for the functions below as well.
The term apiGroup is not correct in this context since we're actually passing in the apiVersion string of the format "group/version".

Also just FYI this format is only for CRDs and apis under the rest path /apis.
The core API group doesn't use this format. e.g for pods apiVersion=v1
https://kubernetes.io/docs/reference/api-overview/#api-groups

The "v1" is actually legacy now. All new APIs will follow the group/version format including CRD and UAS.

I took a look of the api-group definition at here https://kubernetes.io/docs/reference/api-overview/#api-groups.

The API group is specified in a REST path and in the apiVersion field of a serialized object.

It seems like api-group is equivalent apiVersion. In our case, the operator-sdk has a flag called --api-group. do we want to use that or --api-version instead?

the distinction between apiVersion and apiGroup is not very clear.

I agree with what @hasbro17 said. Let's change based on his suggestion.

…-tini-path Bug 1747376 - Fix tini path in downstream image

fanminshi mentioned this pull request Feb 19, 2018

Action Items #10

Closed

21 tasks

fanminshi mentioned this pull request Feb 20, 2018

[WIP] pkg/generator: add cmd/main.go generator #22

Closed

fanminshi added 3 commits February 20, 2018 12:58

commands: add ExitError and ExitSuccess error codes in error.go

e9e35b2

commands/operator-sdk: support dir stucture generation in cmd/new.go

6d2a016

pkg/generator: add default directories generation

ceefdc1

hongchaodeng reviewed Feb 20, 2018

View reviewed changes

*: change gen output

c1373f7

hongchaodeng reviewed Feb 20, 2018

View reviewed changes

hasbro17 reviewed Feb 20, 2018

View reviewed changes

gen: add ascii graph for render output

31c32cf

hongchaodeng merged commit d255e8e into operator-framework:master Feb 20, 2018

hongchaodeng deleted the gen_dirs branch February 20, 2018 22:53

hasbro17 reviewed Feb 20, 2018

View reviewed changes

fanminshi mentioned this pull request Feb 20, 2018

discuss: difference between apiVersion and apiGroup #27

Closed

m1kola pushed a commit to m1kola/operator-sdk that referenced this pull request Jun 7, 2024

Merge pull request operator-framework#21 from fabianvf/fix-downstream…

cd06f7a

…-tini-path Bug 1747376 - Fix tini path in downstream image

Conversation

fanminshi commented Feb 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fanminshi commented Feb 19, 2018

Uh oh!

hongchaodeng commented Feb 19, 2018

Uh oh!

fanminshi commented Feb 19, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hongchaodeng commented Feb 20, 2018

Uh oh!

fanminshi commented Feb 20, 2018

Uh oh!

fanminshi commented Feb 20, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fanminshi commented Feb 20, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hongchaodeng commented Feb 20, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

fanminshi commented Feb 19, 2018 •

edited

Loading