Port Ansible scaffolding to Kubebuilder plugin

[asmacdo]

Boilerplate

• Create init plugin
• Create api plugin

Config

• Scaffold kustomize default dir
• Scaffold CRD bases and kustomization
• Scaffold manager
• Scaffold RBAC
• Scaffold samples (CR)

Operator Image

• Scaffold Dockerfile
• Scaffold ansible Roles
• scaffold requirements.yaml
• scaffold watches.yaml

Misc

• Indicate modified in kubernetes copyright
• Docs or file issue Will be added in a followup
• Pass metrics args and leader election args to manager

Tests

• Port e2e tests
• scaffold molecule

[camilamacedo86]

what is patchesJson6902?

[fabianvf]

It's the way to specify a JSON merge patch https://kubectl.docs.kubernetes.io/pages/reference/kustomize.html#patchesjson6902

[camilamacedo86]

Oh, very interesting. But, why do we need to do this change? What is the end goal?
What is the problem that you are trying to solve with?

[fabianvf]

It allows us to add RBAC for CRDs without modifying the central role.yaml that the user configures, and means we don't need an updater for the role.yaml

[camilamacedo86]

But then you are changing the common the purposed of these files. They should not be used by us. They are just helpers for the admins. I'd recommend to check the role implementation in helm. I think it would be exactly the same for ansible.

[camilamacedo86]

hi @asmacdo,

Could you please revert the changes in the structure of the files and not move them for other directories?

See that, when something change in Go/Kube we should be able to easily apply/patch the changes if we are able to diff both plugins dirs. The same across Helm/Ansible since they are very similar. Also, note that the changes made in the directories of the common files make harder not only to keep it maintained and synchronized across the projects with the other types as to review this pr as well. In this way;

• As a maintainer, I'd like to diff ansible/helm plugins dirs and get just the diff between both. All common files would be ==
• As a maintainer, I'd like to diff go(kb)/ansible plugins dirs and get just the diff between both. All common files would be ==

[fabianvf]

@camilamacedo86 we intentionally changed the directory structure of the Ansible scaffolding to match the directory structure of the scaffolded code to make maintenance and contribution easier (especially from our less go-savvy Ansible contributors), these projects are likely going to be split into their own repos soon so I don't think there's much advantage to a 1-1 match up in directory structures.

[estroz]

Is this being set to true because the type's spec/status will change when fields are added? If so, do you think it makes sense to remove these to enforce validation on all fields by default? If the operator author wants to skip validation, they can manually add these.

[fabianvf]

Adding validation is painful in Ansible as it has to be done entirely by hand, until we have some kind of tooling to support it I think it makes sense to be permissive by default. Anyone going through the trouble of writing the whole schema by hand can also remove this field in the process if they'd like to

[estroz]

That's fair, just gotta make sure this is documented since the CRD scaffold behavior differs from Go's.

[estroz]

From the Helm plugin:

Suggested change

	KUSTOMIZE=./bin/kustomize
	KUSTOMIZE=$(realpath ./bin/kustomize)

[estroz]

Suggested change

	# chmod +x ./bin/helm-operator ;\
	# chmod +x ./bin/ansible-operator ;\

[estroz]

Do you plan on removing this panic? Ideally the watches fragment could be constructed without using a template as to not have to handle an error. If that is not possible/ergonomic, I'd rather see this template executed in the caller and passed as a field to WatchesUpdater. Perhaps in an exported function that generates a watches fragment?

[asmacdo]

I had to use a template rather than Sprintf to enable {{.Resource.Kind | lower }} in the fragment. I borrowed the error handling from a similar situation in the helm plugin

I assume the reason for moving this into an exported function is that it would improve error handling?

[estroz]

Pretty much. I'd like to avoid panics if possible, and this function technically could panic because it takes user input (Resource), however unlikely due to upstream validation. I'm fine leaving this alone for now with a TODO and addressing in a follow-up.

[camilamacedo86]

IMO the watches here would be == the helm one. However, instead of you have the Charts attribute you will have the ansible role and playbooks.

[camilamacedo86]

Hi @fabianvf,

we intentionally changed the directory structure of the Ansible scaffolding to match the directory structure of the scaffolded code to make maintenance and contribution easier (especially from our less go-savvy Ansible contributors)

How move the path or rename the common(base core) files can be helpful for them? For example; why/how move the auth_proxy_patch.go (which is == for all types) from/scaffolds/templates/metricsauth/auth_proxy_patch.go to templates/config/kustomize/auth_proxy_patch.go make maintenance and contribution easier ?

these projects are likely going to be split into their own repos soon so I don't think there's much advantage to a 1-1 match up in directory structures.

Let's imagine the very common scenarios;

• A new version of Go/kubebuilder was released and we need to patch the changes to ansible/helm
• A new feature or fix for ansible that should be applied to Helm
• A new feature or fix for helm that should be applied to ansible

Note that, we can just diff <repoA>/<pluginA>/<versionA> with <repoB>/<pluginB>/<versionB> via many tools to check what, where and how was changed which allows us easily decide what should be applied or not and do not matter if the dirs are or not in the same repo.

ps.: Indeed, just look for the same file and remember that we need to memorize that it is in A and B in one place when is another in C increase the complexity unnecessarily in mine POV.

• I am as a maintainer, I'd like to diff go/ansible/helm plugins dirs and get just the diff between them. All common/core base-files would be equals.

So IMO it is very important we keep the same structure and do not change the default/common files across them. (At least in this first moment, note that we may realize that we ought to centralize it)

@jmrodri @joelanford @estroz wdyt?
c/c @asmacdo

[camilamacedo86]

IMO this file is not required see that each const is used just once in the boilerplate that will use that to scaffold the file,

[camilamacedo86]

Suggested change

	if !p.doAPIScaffold {
	fmt.Printf("Next: define a resource with:\n$ %s create api\n", p.commandName)
	}
	return nil
	// runs the SDK customizations (wrappers)
	if err := utilplugins.UpdateMakefile(p.config); err != nil {
	return err
	}
	if p.doAPIScaffold {
	return p.apiPlugin.PostScaffold()
	}
	fmt.Printf("Next: define a resource with:\n$ %s create api\n", p.commandName)
	return nil

[camilamacedo86]

runs the customization for SDK

[estroz]

I'll handle this in a follow-up.

[camilamacedo86]

Suggested change

	pluginVersion = plugin.Version{Number: 1}
	pluginVersion = plugin.Version{Number: 1}
	pluginKey = plugin.KeyFor(Plugin{})

[estroz]

pluginKey wouldn't be used anywhere yet. I expect this to be added when help text is.

[jmrodri]

/lgtm

[joelanford]

/lgtm

[estroz]

/lgtm

[openshift-ci-robot]

New changes are detected. LGTM label has been removed.

[estroz]

Suggested change

	operator-sdk init --plugins ansible.operator-sdk.io/v1 \
	operator-sdk init --plugins ansible.sdk.operatorframework.io/v1 \

[estroz]

Suggested change

	operator-sdk init --plugins ansible.operator-sdk.io/v1 \
	operator-sdk init --plugins ansible.sdk.operatorframework.io/v1 \

The remaining requested changes will be addressed in follow-ups