Align Helm/Ansible plugins using common/v3 base #4701

camilamacedo86 · 2021-03-25T18:30:51Z

Description

Align Helm/Ansible with Go by making use of the common base and by removing the duplications
Align SDK with KB
Add configComponent feature for Ansible/Helm
Add a new plugin option to let users create only the common base with sdk

Motvation
Closes: #4542
Closes: #4643

Blocked by
kubernetes-sigs/kubebuilder#2106

hack/generate/samples/internal/ansible/memcached.go

hack/generate/samples/internal/ansible/memcached_molecule.go

Adirio · 2021-03-26T06:15:42Z

internal/cmd/operator-sdk/cli/cli.go

 			gov2Bundle,
 			gov3Bundle,
 			helmBundle,
+			commonv3.Plugin{},


I would not include this for now as it is not used for anything and it will imply backwards compatibility issues if removed later.

It is fine. It is only to show @jmrodri the possibility of something that we spoke about. That would be in its own pr with my wish/idea of adding a Plugin Contribution doc with the vision of plugins. Also, we need/can add the custom plugin provide by kB ( the declarative.go.kubebuilder.io/v1: pattern too to allow people to get the advantage of this one and custom their implementation )

It allows people such as Java developers who want to create their own plugin to follow up the same base structure and then scaffold the rest with their language of preference instead of using Golang.

internal/plugins/ansible/v1/init.go

internal/testutils/olm.go

Adirio · 2021-03-26T06:26:42Z

internal/util/utils.go

+	"strings"
+)
+
+func ReplaceInFile(path, old, new string) error {


As we are going to use this functions to scaffold, they should receive a filesystem.Filesystem and use afero internally instead of ioutil or os. If we use these others directly, once we implement improvements in the filesystem abstraction (for example the scaffold atomicity we talked about) these methods will fail.

We can do that in a follow-up. These funcs exist in KB https://github.com/kubernetes-sigs/kubebuilder/blob/master/test/e2e/utils/util.go#L63 but are not returning the errors. (needs improvements see my todo in this file)

Note that there or here we need to Insert/Replace data in the files to customize the plugins. So, IMO it can be promoted as utils from KB. Then, we can check the possibility to change that to use afero.

Also, see my comment in : #4701 (comment)

Also see that we have a very complex code in the manifests plugin that could be simplified with that.

But before this PR, and in the KB package you mentioned, they are used during tests after running the correspondig command call, which means that the file exists in disk.

However, here you are using these functions during scaffolding, and all scaffolding should be done through the filesystem abstraction, not directly with os and ioutil packages. Thats the point I wanted to make. I know the current machinery only allows to create files from templates, or insert code-fragments before special markers. Do we need to improve the machinery for generalized insertion and replacement? Yes. But until that is done, SDK will have to rely on these functions, and scaffolding directly to the filesystem without using the abstraction provided should not be done.

It is not about the file. It is about the content not be found. The same issue can happen there indeed in the tests. So, for example, the test will pass because the required change has not been performed.

However, I am totally fine with the improvements sugggested getting done in follow ups. the intention here was just to share the scenario.

I'm not talking about the logic if the content is not found Camila, I'm talking about the filesystenm abstraction. We are providing one common filesystem abstraction that all Scaffold() must use. This allows us to, for example, test by saying that the underlying filesystem is an afero.MemMapFs. If we do this, when it gets to this scaffold, the part that uses the system abstraction will work, but the direct calls to os and ioutil will break. The files that you are trying to insert into will only be created in memory while os and ioutil calls will expect it to be in disk.

If you want to use these functions inside Scaffold and PreScaffold methods, you must turn those calls to os and ioutil functions to the equivalents of afero and pass the filesystem abstraction. If you only want to use them for tests (like until now) you don't have this problem. After moving everything to afero, when you call this function from tests that don't have a filesystem you just create one like so: filesystem.Filesystem{FS: afero.NewOsFs()}.

Could not it be done in a follow-up? Could not do this after we improve these implementations in upstream/kb and provide them from there? Is that a blocker for us to move forward here?

I fear it should block, but should be pretty easy:

Add fs filesystem.Filesystem as an argument.

When calling this function from inside Scaffold pass the fs that you have there.

When calling this function from any other place (tests) pass it a filesystem.Filesystem(FS: afero.NewOsFs()).

When using ioutil.Xxxxxx(...) change it to afero.Xxxxxx(fs.FS, ...).

When using os.Xxxxxx(....) change it to fs.FS.Xxxxxx(...).

What bug/issue we will face that justify we block this PR instead of address that in follow up when your pr kubernetes-sigs/kubebuilder#2119 get merged? I do not think so. So, we can do that as follow up. I will add the todo comments.

internal/util/utils.go

test/integration/suite_test.go

camilamacedo86 · 2021-03-26T10:25:40Z

internal/plugins/envtest/v1/init.go

-	makefileBytes = []byte(strings.Replace(string(makefileBytes), oldMakefileTestTarget, fmt.Sprintf(makefileTestTarget, controllerRuntimeVersion), 1))
-
-	var mode os.FileMode = 0644
-	if info, err := fs.FS.Stat(filePath); err == nil {


@Adirio , regarding the code implementation, suggested which I am ok within a follow-up. See that it was NOT raising the issue when the target is not found as it is in the Insert/replace functions that we used. Just to let, you know.

camilamacedo86 · 2021-03-29T18:30:11Z

internal/cmd/operator-sdk/cli/cli.go

 	)
-	gov3Bundle, _ := plugin.NewBundle(golangv3.Plugin{}.Name(), golangv3.Plugin{}.Version(),
+	gov3Bundle, _ := plugin.NewBundle(golang.DefaultGoNameQualifier, golangv3.Plugin{}.Version(),
+		commonv3.Plugin{},


We might not need to use the common here. see: kubernetes-sigs/kubebuilder#2112

In order to do this, we need to export the bundle, which I have planned to do once kubernetes-sigs/kubebuilder#2106 is merged. (I'm not doing it right now to prevent rebases)

we can do that in a follow-up. I will add a comment

jmrodri

/lgtm

jmrodri · 2021-04-07T15:59:12Z

/lgtm

Signed-off-by: Camila Macedo <[email protected]>

internal/plugins/ansible/v1/scaffolds/internal/templates/config/kdefault/kustomization.go

 	f.TemplateBody = kustomizeTemplate

-	f.IfExistsAction = machinery.Error
+	// For Anible/Helm is no supported webhooks then, we customize


internal/plugins/helm/v1/scaffolds/internal/templates/config/kdefault/kustomization.go

 	f.TemplateBody = kustomizeTemplate

-	f.IfExistsAction = machinery.Error
+	// For Anible/Helm is no supported webhooks then, we customize


estroz · 2021-04-07T16:49:17Z

internal/plugins/ansible/v1/init.go

+        env:
+          - name: ANSIBLE_GATHERING
+            value: explicit`


Small nit to align YAML formatting

Suggested change

env:

- name: ANSIBLE_GATHERING

value: explicit`

env:

- name: ANSIBLE_GATHERING

value: explicit`

estroz

LGTM other than the formatting nit

/lgtm

openshift-ci-robot · 2021-04-07T17:52:59Z

New changes are detected. LGTM label has been removed.

Signed-off-by: Camila Macedo <[email protected]>

Update related to v1.6.1 https://sdk.operatorframework.io/docs/upgrading-sdk-version/v1.6.1/ operator-framework/operator-sdk#4701 Signed-off-by: Wayne Sun <[email protected]>

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 Mar 25, 2021

openshift-ci-robot requested review from fabianvf and marc-obrien March 25, 2021 18:30

camilamacedo86 temporarily deployed to deploy March 25, 2021 18:31 Inactive

camilamacedo86 temporarily deployed to deploy March 25, 2021 20:32 Inactive

camilamacedo86 temporarily deployed to deploy March 25, 2021 20:57 Inactive

camilamacedo86 temporarily deployed to deploy March 25, 2021 20:58 Inactive

camilamacedo86 temporarily deployed to deploy March 25, 2021 21:54 Inactive

camilamacedo86 changed the title ~~WIP - test~~ Align Helm/Ansible plugins with the changes made for Golang ( go/v3 ) Mar 25, 2021

camilamacedo86 temporarily deployed to deploy March 25, 2021 22:51 Inactive

camilamacedo86 temporarily deployed to deploy March 26, 2021 00:02 Inactive

Adirio suggested changes Mar 26, 2021

View reviewed changes

camilamacedo86 commented Mar 26, 2021

View reviewed changes

camilamacedo86 commented Mar 29, 2021

View reviewed changes

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 5, 2021

jmrodri approved these changes Apr 7, 2021

View reviewed changes

openshift-ci-robot assigned jmrodri Apr 7, 2021

openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. lgtm Indicates that a PR is ready to be merged. labels Apr 7, 2021

camilamacedo86 marked this pull request as ready for review April 7, 2021 15:55

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 Apr 7, 2021

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 7, 2021

align with kb to use common plugin instead of duplicate the code

c9017c2

Signed-off-by: Camila Macedo <[email protected]>

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Apr 7, 2021

estroz reviewed Apr 7, 2021

View reviewed changes

internal/plugins/helm/v1/scaffolds/internal/templates/config/kdefault/kustomization.go

f.TemplateBody = kustomizeTemplate

f.IfExistsAction = machinery.Error

// For Anible/Helm is no supported webhooks then, we customize

This comment was marked as outdated.

Sign in to view

estroz reviewed Apr 7, 2021

View reviewed changes

estroz approved these changes Apr 7, 2021

View reviewed changes

openshift-ci-robot assigned estroz Apr 7, 2021

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 7, 2021

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Apr 7, 2021

address suggestion review nit

742fc0c

Signed-off-by: Camila Macedo <[email protected]>

camilamacedo86 merged commit 2a1bd03 into operator-framework:master Apr 7, 2021

camilamacedo86 deleted the plugins-kb branch April 7, 2021 19:21

Align Helm/Ansible plugins using common/v3 base #4701

Align Helm/Ansible plugins using common/v3 base #4701

Uh oh!

Conversation

camilamacedo86 commented Mar 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

camilamacedo86 Mar 26, 2021 • 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!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

camilamacedo86 Mar 26, 2021 • 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!

Adirio Mar 26, 2021 • 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!

Adirio Mar 26, 2021 • 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!

camilamacedo86 Mar 26, 2021 • 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!

camilamacedo86 Apr 7, 2021 • 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!

Uh oh!

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!

jmrodri left a comment

Choose a reason for hiding this comment

Uh oh!

jmrodri commented Apr 7, 2021

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

estroz left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ci-robot commented Apr 7, 2021

Uh oh!

Reviewers

Assignees

Labels

camilamacedo86 commented Mar 25, 2021 •

edited

Loading

camilamacedo86 Mar 26, 2021 •

edited

Loading

camilamacedo86 Mar 26, 2021 •

edited

Loading

Adirio Mar 26, 2021 •

edited

Loading

Adirio Mar 26, 2021 •

edited

Loading

camilamacedo86 Mar 26, 2021 •

edited

Loading

camilamacedo86 Apr 7, 2021 •

edited

Loading