commands/operator-sdk: Add CLI support for Ansible Operator by dymurray · Pull Request #486 · operator-framework/operator-sdk

dymurray · 2018-09-14T18:52:56Z

operator-sdk new --type ansible
operator-sdk build support. Autodetect type of operator by looking for watches.yaml
operator-sdk generate support.
~~[ ] operator-sdk up local support.~~ Going to break this out to a separate PR

shawn-hurley

Some comments even though this is WIP

shawn-hurley · 2018-09-14T18:58:33Z

 }
+
+func determineOperatorType() string {
+	if _, err := os.Stat(watchesYaml); err == nil {


@dymurray I would prefer this to be if there is a go file in the cmd dir rather than if a watches file exists.

The reason is, if there is a go the main file, we should build that, if there is not then we assume ansible. This will be less error-prone and should handle other use cases like building a go operator but using some of the ansible operators.

On the other hand, we probably shouldn't assume that the lack of go source means ansible. Other types, like perhaps helm, may not have go source either. It could be best to positively identify one or the other, and let the default be "error; no operator type detected".

Considering all that happens now, is a build of the dockerfile, that seems like the reasonable default IMO. If we add a third type we can deal with the differences then IMO.

Need to change this so that we first check for cmd/<name>/main.go and if we find it we build just like we expect for a normal golang operator except with the Ansible Operator Dockerfile.

shawn-hurley · 2018-09-14T19:02:09Z

 	newCmd.MarkFlagRequired("kind")
+	newCmd.Flags().StringVar(&operatorType, "type", "go", "Type of operator to initialize (e.g \"ansible\")")
 	newCmd.Flags().BoolVar(&skipGit, "skip-git-init", false, "Do not init the directory as a git repository")
+	newCmd.Flags().BoolVar(&generatePlaybook, "generate-playbook", false, "Generate a playbook skeleton in watches.yaml for Ansible Operator")


Mention that this is only used for type ansible.

shawn-hurley · 2018-09-14T19:05:01Z

 	}
-	pullDep()
-	generate.K8sCodegen(projectName)
+	if operatorType == "go" {


This is the second time we are comparing this value, might as well make it a constant?

shawn-hurley

@dymurray I am wondering if we may want to create multiple generator(common/ansible/go) types with a Generator interface. This way we do not have switch statements in multiple places but rather a type that encapsulates generating that type.

WDYT?

/cc @hasbro17 @fanminshi @mhrivnak

shawn-hurley · 2018-09-21T13:41:48Z

+	crdCmd := &cobra.Command{
+		Use:   "crd",
+		Short: "Generates a custom resource definition",
+		Long: `generates a custom resource definition (CRD) file for the .


Are we missing the end of this?

oops... yes.

shawn-hurley · 2018-09-21T13:49:22Z

+		Version:      version(apiVersion),
+	}
+	crdFilePath := filepath.Join(deployDir, strings.ToLower(kind) + "_crd.yaml")
+	return renderWriteFile(crdFilePath, crdFilePath, crdYamlTmpl, crdTd)


I think this should create a cr.yaml for the new CRD as well, IMO.

ok. i will update to add cr.yaml as well

shawn-hurley · 2018-09-21T13:49:48Z

+	return renderWriteFile(crdFilePath, crdFilePath, crdYamlTmpl, crdTd)
+}
+
+func renderDeployFiles(deployDir, projectName, apiVersion, kind, operatorType string) error {


this should use the RenderDeployCrdFile IMO

shawn-hurley · 2018-09-21T14:07:54Z

+	fmt.Fprintln(os.Stdout, "Generating custom resource definition (CRD) file")
+
+	// Generate CRD file
+	if err := generator.RenderDeployCrdFile(apiVersion, kind); err != nil {


Is there a way to verify that we are in the project root when running this command? I am a little worried that we are not guarding against that, and could have some odd behavior for the user.

agreed. I've made an update to check for the ./deploy sub-directory

dymurray · 2018-09-21T15:08:04Z

@shawn-hurley That makes sense to me. Especially if operator-sdk is going to be expanded to encapsulate other types of operators. Some of these functions however I felt that it was useful to encapsulate everything in the same place. Rendering tmp was one example... but curious what others think as well.

mhrivnak · 2018-09-21T15:17:41Z

 	newCmd.MarkFlagRequired("kind")
+	newCmd.Flags().StringVar(&operatorType, "type", "go", "Type of operator to initialize (e.g \"ansible\")")
 	newCmd.Flags().BoolVar(&skipGit, "skip-git-init", false, "Do not init the directory as a git repository")
+	newCmd.Flags().BoolVar(&generatePlaybook, "generate-playbook", false, "Generate a playbook skeleton in watches.yaml. (Only used for --type ansible)")


Should this say "and watches.yaml"? A playbook skeleton would be created separately on the filesystem, and then referenced from within watches.yaml. It may be confusing to say the skeleton of a playbook will be in watches.yaml.

Good catch. Your wording is far more accurate.

Actually... I'm going to remove and watches.yaml and just leave it at Generate a playbook skeleton since we always generate watches.yaml when --type ansible.

mhrivnak · 2018-09-21T15:43:03Z

+			return err
+		}
+	default:
+		return errors.New(fmt.Sprintf("unexpected operator type [%v]", g.operatorType))


Check out fmt.Errorf()

dymurray · 2018-09-21T17:35:54Z

@johnkim76 You might be best to answer Shawn's questions above

hasbro17 · 2018-09-21T22:21:01Z

I am wondering if we may want to create multiple generator(common/ansible/go) types with a Generator interface. This way we do not have switch statements in multiple places but rather a type that encapsulates generating that type.

@shawn-hurley Yes we'll definitely need a more general generator/scaffolder.
Fanmin and I discussed this while thinking about the controller-runtime refactor.
We've held off on this right now but I kind of like the approach in controller-tools where they have a general scaffolding utility that can scaffold any kind of file based on the input struct(template+values+file-location).
https://github.com/kubernetes-sigs/controller-tools/blob/master/cmd/controller-scaffold/cmd/project.go#L86-L97
https://github.com/kubernetes-sigs/controller-tools/blob/master/pkg/scaffold/scaffold.go#L133-L179

Right now we're only coupling the templates and their inputs structs together and just generate each file individually i.e we don't use a generator, but we'll eventually have to add a more general layer to handle that for any kind of file().
e.g here's how we generate a file right now.
https://github.com/operator-framework/operator-sdk/blob/refactor/controller-runtime/pkg/scaffold/register.go
https://github.com/operator-framework/operator-sdk/blob/refactor/controller-runtime/commands/operator-sdk/cmd/add/api.go#L112-L121

With a more general generator/scaffolder we could just change the inputs (based on the CLI command and type of operator) and generate the necessary files.

But we'll have to think that through a bit more and not in this PR I think.

dymurray · 2018-09-24T15:34:57Z

@shawn-hurley @hasbro17 In response to your comments, is there anything you'd like me to change about my implementation in the scaffolding logic? Or is this something that you'd like to address at a later time?

shawn-hurley · 2018-09-24T16:00:31Z

I think @hasbro17 wants to deal with it later, so no changes.

shawn-hurley · 2018-09-26T19:03:30Z

@dymurray when you get a chance could you please rebase?

fanminshi · 2018-09-28T19:57:08Z

lgtm after nits

shawn-hurley · 2018-10-01T12:09:38Z

@hasbro17 was your review complete and does this now look good to you?

hasbro17 · 2018-10-01T18:16:42Z

@shawn-hurley Sorry I got busy with a couple of other reviews. I'll finish reviewing this today.

hasbro17 · 2018-10-01T21:25:31Z

-	if err != nil {
-		cmdError.ExitWithError(cmdError.ExitError, fmt.Errorf("failed to build: (%v)", string(o)))
+	// Don't need to buld go code if Ansible Operator
+	if buildCmd() {


Can you change the name to be more representative of the actual function?
Right now it's just checking if cmd/<project-name>/main.go exists to determine if it's an Ansible or Go type project.
So perhaps mainExists() or isGoProject().

hasbro17 · 2018-10-01T21:30:17Z

+	fmt.Fprintln(os.Stdout, "Generating custom resource definition (CRD) file")
+
+	// generate CRD file
+	wd, _ := os.Getwd()


Check return error.

hasbro17 · 2018-10-01T21:51:23Z

+		Short: "Generates a custom resource definition (CRD) and the custom resource (CR) files",
+		Long: `The operator-sdk generate command will create a custom resource definition (CRD) and the custom resource (CR) files for the specified api-version and kind.
+
+Generated CRD filename: <project-name>/deploy/<kind>_crd.yaml


@shawn-hurley Just wanted to point out that if you try to generate a second CRD with the same Kind but different APIVersion then that will overwrite the previous <kind>_crd.yaml. Not sure if that's a legitimate use case in an ansible type project but we account for multiple CRDs in the refactor branch by using the format deploy/crds/<group>_<version>_<kind>_crd.yaml

https://github.com/operator-framework/operator-sdk/blob/refactor/controller-runtime/commands/operator-sdk/cmd/add/api.go#L146-L168

We'll soon add the changes from this PR into the refactor branch as well(to fit the style of the scaffold).
Right now we don't have a separate generate CRD command but I'm wondering if you wanted to make the CRD generation consistent with how we do it in the refactor branch.

This makes sense to me... @johnkim76 any concerns around this?

that sounds good. I'll make the update to the filename. thank you.

hasbro17 · 2018-10-01T22:03:12Z

+// │   │   └── <kind>
+// │   ├── watches.yaml
+// │   ├── deploy
+// │   │   ├── <kind>-CRD.yaml


Minor nit but unless I'm mistaken, operator-sdk new --type=ansible --api-version=<apiversion> --kind=<kind> will still generate:

// │ ├── deploy // │ │ ├── crd.yaml // │ │ ├── rbac.yaml // │ │ ├── operator.yaml // │ ├ └── cr.yaml

<kind>_crd.yaml and <kind>_cr.yaml are only generated by operator-sdk generate crd --kind --api-version.
See renderDeployFiles() below. Although that should still be fine I think.

@johnkim76 is this intended? Would you prefer that we change the new output to have the same filenames?

yes, I think we should change the new behavior to have the <g>_<v>_<k>_cr.yaml and <g>_<v>_<k>_crd.yaml names.

@johnkim76 based on Shawn's comment below we are going to hold off on that for the refactoring.

hasbro17 · 2018-10-01T22:12:18Z

+func (g *Generator) renderRole() error {
+	fmt.Printf("Rendering Ansible Galaxy role [%v/%v/%v]...\n", g.projectName, rolesDir, g.kind)
+	agcmd := exec.Command(filepath.Join(g.projectName, galaxyInitCmd))
+	_, err := agcmd.CombinedOutput()


It might be worth logging or returning the CombinedOutput here as well if the command fails.
The galaxyInit script might run but exit with an error. In which case the stdout and stderr might be more useful to see.
The err returned will just only say that the command exited with exit code 1.

hasbro17 · 2018-10-01T22:24:05Z

LGTM after nits.

@shawn-hurley We don't have to address the changes to the generate crd command right now but I thought I would just mention that we might have to change the output later on as we unify these changes with the layout from the refactor branch.

dymurray · 2018-10-02T12:47:45Z

Thanks a lot for the feedback @hasbro17. @johnkim76 I would like your thoughts on the generate crd feedback provided. Not sure what you would prefer we do.

shawn-hurley · 2018-10-02T13:31:32Z

@dymurray @johnkim76 I think that we can fix the filenames for the generation command now, and then deal with the new command in the refactor branch as @hasbro17 suggested.

I think this will work better because we shouldn't be changing new for everyone unless it is on the refactor branch IMO and fixing the CRD generation is new and shouldn't have expectations around it already.

Does this make sense to everyone?

shawn-hurley · 2018-10-02T13:34:26Z

+
+// verifyCrdDeployPath checks if the path <project-name>/deploy sub-directory is exists, and that is rooted under $GOPATH
+func verifyCrdDeployPath() {
+	// check if $GOPATH env exists


While re-looking at the code, I would like to change one more thing about the generation as well

I think an ansible operator new, should not have to have Gopath set up. I think this would force users to have that, and I think we should avoid this.

@johnkim76 I wonder if we should just check that the current working dir has a deploy directory instead?

ok.. I'll remove the check for the Gopath

Good catch Shawn. I agree we shouldn't depend on Go being configured.

…perator-framework#473)

…ator-framework#546) * commands,pkg/test: support single namespace mode for local test This commit brings support for running the tests in a single namespace when using `test local`, which was previously only used by `test cluster` (where it was a requirement to properly function) * doc: update docs with new test local --namespace flag * pkg/test/resource_creator.go: check namespace env value * pkg/test: change handling of TestNamespaceEnv

…or-framework#548) PR operator-framework#525 changed the way that dep worked to fix dependency issues when doing e2e tests on travis. The PR breaks e2e testing on local machines though, as the tests will just use the master branch instead of a local branch. This commit symlinks the local sdk into vendor on non-travis tests. The tests will still fail on local machines if there is a dependency change, but we are unable to fix this on local machines unless `dep` adds local repo support.

hasbro17 · 2018-10-02T17:34:10Z

@shawn-hurley Sounds good to me.

shawn-hurley reviewed Sep 14, 2018

View reviewed changes

dymurray changed the title ~~[WIP] Add CLI support for Ansible Operator~~ Add CLI support for Ansible Operator Sep 19, 2018

openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 19, 2018

dymurray changed the title ~~Add CLI support for Ansible Operator~~ [WIP] Add CLI support for Ansible Operator Sep 19, 2018

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 19, 2018

dymurray force-pushed the ansiblecli branch 2 times, most recently from 6df8bfe to d240c1e Compare September 20, 2018 20:34

openshift-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 20, 2018

dymurray changed the title ~~[WIP] Add CLI support for Ansible Operator~~ Add CLI support for Ansible Operator Sep 20, 2018

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 20, 2018

shawn-hurley requested changes Sep 21, 2018

View reviewed changes

openshift-ci-robot requested review from fanminshi, hasbro17 and mhrivnak September 21, 2018 14:26

mhrivnak reviewed Sep 21, 2018

View reviewed changes

johnkim76 force-pushed the ansiblecli branch from 41ce6d7 to 2df7785 Compare September 21, 2018 21:08

johnkim76 force-pushed the ansiblecli branch 2 times, most recently from 529f74a to 5286eb3 Compare September 22, 2018 03:51

shawn-hurley approved these changes Sep 24, 2018

View reviewed changes

shawn-hurley added the language/ansible Issue is related to an Ansible operator project label Sep 26, 2018

dymurray force-pushed the ansiblecli branch 2 times, most recently from c371dcb to 310544f Compare September 26, 2018 20:23

Fix merge conflict in galaxy script

b10bc85

shawn-hurley changed the title ~~Add CLI support for Ansible Operator~~ commands/operator-sdk: Add CLI support for Ansible Operator Sep 27, 2018

fanminshi reviewed Sep 28, 2018

View reviewed changes

Comment thread commands/operator-sdk/cmd/generate/crd.go

Nit update

15fe320

hasbro17 reviewed Oct 1, 2018

View reviewed changes

Nit updates based on feedback

6ba4d7e

shawn-hurley requested changes Oct 2, 2018

View reviewed changes

Shawn Hurley and others added 5 commits October 2, 2018 10:03

pkg/ansible: Adding ansible operator controller and events package (o…

249d7b1

…perator-framework#473)

make ansible task log outputs more readable (operator-framework#545)

02f1155

update as suggested in comments

096ae3a

openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 2, 2018

dymurray added 2 commits October 2, 2018 11:36

Do not generate service account file for Ansible Operator

a47f90f

Update test

1962e6b

shawn-hurley approved these changes Oct 2, 2018

View reviewed changes

leifmadsen mentioned this pull request Oct 2, 2018

docs: Add a user guide for using an Ansible Operator #559

Merged

4 tasks

shawn-hurley merged commit a947a51 into operator-framework:master Oct 2, 2018

Conversation

dymurray commented Sep 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shawn-hurley left a comment

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!

shawn-hurley Sep 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Uh oh!

shawn-hurley left a comment

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!

Uh oh!

dymurray commented Sep 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

dymurray commented Sep 21, 2018

Uh oh!

hasbro17 commented Sep 21, 2018

Uh oh!

dymurray commented Sep 24, 2018

Uh oh!

shawn-hurley commented Sep 24, 2018

Uh oh!

shawn-hurley commented Sep 26, 2018

Uh oh!

Uh oh!

fanminshi commented Sep 28, 2018

Uh oh!

shawn-hurley commented Oct 1, 2018

Uh oh!

hasbro17 commented Oct 1, 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!

hasbro17 Oct 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

dymurray commented Sep 14, 2018 •

edited

Loading

shawn-hurley Sep 14, 2018 •

edited

Loading

dymurray commented Sep 21, 2018 •

edited

Loading

hasbro17 Oct 1, 2018 •

edited

Loading