Kubebuilder layout ansible tests, by Fabian by asmacdo · Pull Request #3433 · operator-framework/operator-sdk

asmacdo · 2020-07-15T15:52:15Z

No description provided.

camilamacedo86

Could we not run the commands and create the mock project to do the test instead of adding the files? e.g https://github.com/operator-framework/operator-sdk/blob/5725a5e3a90e2f2b580a7774b4e4d39e17335e0b/hack/tests/e2e-helm.sh

Otherwise, we are not acctually testing what is scaffold by the tool as well.

fabianvf · 2020-07-15T18:52:47Z

@camilamacedo86 these tests are not for scaffolding but for more advanced integration tests of Ansible + ansible-runner + the kubernetes modules, there's another test (that's not updated by this PR) that tests the scaffolded code

camilamacedo86 · 2020-07-15T19:10:20Z

Hi @fabianvf,

I do see too much the point of we have tests in the project that are not executed from the code which is gen via the tool. However, if it is required for we move forward and have the Ansible plugin I am 100% OK with. See that with kbutils we can inject code and manipulating the files in the Go tests. (e.g https://github.com/kubernetes-sigs/kubebuilder/blob/master/test/e2e/v3/e2e_suite.go#L84-L92 and https://github.com/kubernetes-sigs/kubebuilder/blob/master/test/e2e/v3/e2e_suite.go#L124-L158)

Could we track the need to replace this for a generated code via an issue in the repo and a todo in the project/dir to remember? Also, would not be better we put the mock project in a dir named testdata?

^

joelanford · 2020-07-15T19:52:24Z

I am of the same mindset as @camilamacedo86 is. Putting static scaffolded files in the repo isn't the best. That's what test/test-framework is (was?), and it was generally a source of problems. If we can avoid it, it would be nice.

How much differs in this test project vs a vanilla test project? If it isn't much, could we do as Camila suggests and fully-regenerate it during the e2e using the actual plugin code and then applying the changes?

+1 on putting this in a testdata directory.

EDIT: I see we already have test/ansible and this is just updating it to use the new project structure. In that case, I partially rescind this comment. This PR doesn't introduce new test directories. It merely updates existing ones. Carry on!

joelanford

There are still a bunch of legacy files left over. E.g. build/Dockerfile and all of deploy. Are we waiting to remove those until we remove the legacy project scaffolding?

joelanford · 2020-07-15T19:58:01Z

+- group: cache.example.com
+  kind: Memcached
+  version: v1alpha1
+version: "2"


Suggested change

version: "2"

version: "3-alpha"

joelanford · 2020-07-15T19:58:52Z

@@ -0,0 +1,7 @@
+domain: com


Add the layout key/value?

value: layout: ansible.operator-sdk.io/v1

joelanford · 2020-07-15T20:02:16Z

@@ -0,0 +1,85 @@
+


There's a whole bunch of Go-operator related code here.

Is the intention to fully rebuild this directory once the plugin can scaffold it with all of the ansible-specific stuff?

joelanford · 2020-07-15T20:03:38Z

+      # - name: kube-rbac-proxy
+      #   image: gcr.io/kubebuilder/kube-rbac-proxy:v0.5.0
+      #   args:
+      #   - "--secure-listen-address=0.0.0.0:8443"
+      #   - "--upstream=http://127.0.0.1:8080/"
+      #   - "--logtostderr=true"
+      #   - "--v=10"
+      #   ports:
+      #   - containerPort: 8443
+      #     name: https
+      - name: manager
+        args:
+        # - "--metrics-addr=127.0.0.1:8080"
+        # - "--enable-leader-election"


We'll probably want a lot of this re-enabled. Should we add TODOs?

joelanford · 2020-07-15T20:04:59Z

+apiVersion: kustomize.config.k8s.io/v1beta1
+kind: Kustomization
+images:
+- name: controller
+  newName: quay.io/asmacdo/controller
+  newTag: v0.0.1


Are these lines necessary? It looks like the default scaffolded file is usually just the resources field.

camilamacedo86 · 2020-07-15T23:34:44Z

+domain: com
+repo: wut
+resources:
+- group: cache.example.com


It is more than just the group. I think it will be cache.example and the domain would be com

camilamacedo86 · 2020-07-15T23:36:45Z

+  issuerRef:
+    kind: Issuer
+    name: selfsigned-issuer
+  secretName: webhook-server-cert # this secret will not be prefixed, since it's not managed by kustomize


is webhooks supported with the current ansible image?

camilamacedo86 · 2020-07-15T23:41:04Z

+  resources:
+  - configmaps/status
+  verbs:
+  - get
+  - update
+  - patch


it is something that does not exist. Also, for Ansible/Helm we need to add the roles that are required for the current impl/base image. See;

- apiGroups: - monitoring.coreos.com resources: - servicemonitors verbs: - get - create - apiGroups: - apps resourceNames: - "" resources: - deployments/finalizers verbs: - update - apiGroups: - "" resources: - pods verbs: - get - apiGroups: - apps resources: - replicasets - deployments verbs: - get - apiGroups: - example.com resources: - '*' verbs: - create - delete - get - list - patch - update - watch

and : https://github.com/operator-framework/operator-sdk/blob/master/internal/plugins/helm/v1/scaffolds/templates/role.go

estroz · 2020-07-29T17:28:07Z

 	github.com/go-logr/zapr v0.1.1
 	github.com/iancoleman/strcase v0.0.0-20191112232945-16388991a334
 	github.com/kr/text v0.1.0
+	github.com/kubernetes-sigs/kustomize v2.0.3+incompatible // indirect


Where's this coming from?

Just go mod tidy will solve -> it is the reason for the PR fails in the sanity test.

that's a good question, I have no idea why/how that's in there, my bad

camilamacedo86 · 2020-07-29T18:49:22Z

+---
+- name: Build kustomize testing overlay
+  # load_restrictor must be set to none so we can load patch files from the default overlay
+  command: '{{ kustomize }} build  --load_restrictor none .'


Thee CI is failing here. If worked locally and not in the CI is because you have the kustomize locally installed and can perform it directly. But in the CI it will have not.

camilamacedo86 · 2020-07-30T20:49:46Z

+make kustomize
+if [ -f ./bin/kustomize ] ; then
+  KUSTOMIZE="$(realpath ./bin/kustomize)"
+else
+  KUSTOMIZE="$(which kustomize)"
+fi


I think might have a better way.
However, IMO the goal is to ensure that we are doing the same tests with the new layout.
I understand that all these tests will be migrated to GO tests, in this way in POV has no reason to be too precious with :-)

camilamacedo86

/lgtm

terrific great work 🥇

openshift-ci-robot · 2020-07-31T16:30:57Z

New changes are detected. LGTM label has been removed.

Co-authored-by: Eric Stroczynski <estroczy@redhat.com>

camilamacedo86 · 2020-08-05T20:22:38Z

        operator_image: ${OPERATOR_IMAGE:-""}
-        operator_pull_policy: ${OPERATOR_PULL_POLICY:-"Always"}
+        pull_policy: ${OPERATOR_PULL_POLICY:-"Always"}
        kustomize: ${KUSTOMIZE_PATH:-kustomize}


@asmacdo you will need to change here as well. https://github.com/operator-framework/operator-sdk/pull/3654/files :-)

camilamacedo86 · 2020-10-11T18:50:49Z

This PR can be replaced by : #4011

c/c @asmacdo @fabianvf

camilamacedo86

.

camilamacedo86 · 2020-10-30T16:22:01Z

@asmacdo 50% of this pr is addressed already with #4011 and for the advanced scenarios, we have the issue; #4025.

So, since it is outdated and if we close it we still able to use as a helper to achieve the #4025 I think it is fine to close this one. However, feel free to re-open if you think that it should not be close at all.

**Description of the change:** - update advanced mock static scenario with 1.0+ layout (#3433) - (note that we need to ensure that the test works with the new layout) - Fix CI issue (blocker master): `The error was: urllib3.exceptions.MaxRetryError: HTTPSConnectionPool(host='172.30.99.1', port=24443): Max retries exceeded with url: /version (Caused by NewConnectionError('<urllib3.connection.HTTPSConnection object at 0x7f28647cdb38>: Failed to establish a new connection: [Errno 110] Connection timed out',))` in GA. NOTE: The follow up here is to replace the test/ansible static mock with samples to ensure that we are testing it with the latest changes always. See the issue #4025 and its PR : #4312

**Description of the change:** - update advanced mock static scenario with 1.0+ layout (operator-framework#3433) - (note that we need to ensure that the test works with the new layout) - Fix CI issue (blocker master): `The error was: urllib3.exceptions.MaxRetryError: HTTPSConnectionPool(host='172.30.99.1', port=24443): Max retries exceeded with url: /version (Caused by NewConnectionError('<urllib3.connection.HTTPSConnection object at 0x7f28647cdb38>: Failed to establish a new connection: [Errno 110] Connection timed out',))` in GA. NOTE: The follow up here is to replace the test/ansible static mock with samples to ensure that we are testing it with the latest changes always. See the issue operator-framework#4025 and its PR : operator-framework#4312 Signed-off-by: reinvantveer <rein.van.t.veer@geodan.nl>

asmacdo added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 15, 2020

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 Jul 15, 2020

openshift-ci-robot requested review from estroz and jmrodri July 15, 2020 15:52

estroz reviewed Jul 15, 2020

View reviewed changes

Comment thread hack/tests/e2e-ansible-molecule.sh Outdated

camilamacedo86 previously requested changes Jul 15, 2020

View reviewed changes

joelanford reviewed Jul 15, 2020

View reviewed changes

camilamacedo86 reviewed Jul 15, 2020

View reviewed changes

Comment thread test/ansible/Makefile

camilamacedo86 reviewed Jul 15, 2020

View reviewed changes

estroz mentioned this pull request Jul 25, 2020

cmd/operator-sdk,internal: remove legacy CLI, clean up utils #3533

Merged

2 tasks

camilamacedo86 mentioned this pull request Jul 25, 2020

Port the tests in test/ansible for the new layout #3540

Closed

This was referenced Jul 26, 2020

*: remove the add crd subcommand and related utilities #3547

Merged

removal: cleanup over legacy support #3539

Merged

cmd/operator-sdk: remove build subcommand #3566

Merged

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

fabianvf force-pushed the ansible-test-kb-layout branch from 1c1c598 to 794eda6 Compare July 29, 2020 16:44

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 29, 2020

fabianvf force-pushed the ansible-test-kb-layout branch from 794eda6 to 73c5963 Compare July 29, 2020 16:53

estroz reviewed Jul 29, 2020

View reviewed changes

camilamacedo86 reviewed Jul 29, 2020

View reviewed changes

camilamacedo86 reviewed Jul 30, 2020

View reviewed changes

camilamacedo86 approved these changes Jul 30, 2020

View reviewed changes

openshift-ci-robot assigned camilamacedo86 Jul 30, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 30, 2020

fabianvf force-pushed the ansible-test-kb-layout branch from dc6568d to 26a7661 Compare July 31, 2020 16:30

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jul 31, 2020

asmacdo and others added 8 commits July 31, 2020 13:36

Kubebuilder layout ansible tests, by Fabian

6cae304

Update hack/tests/e2e-ansible-molecule.sh

172bcfa

Co-authored-by: Eric Stroczynski <estroczy@redhat.com>

Update for newest scaffolding

7bf666d

Get all tests passing

35757d8

Fix missing kustomize

e3b3419

operator_pull_policy -> pull_policy

18a8b07

Pull in new argstest

ba92493

Fix rebase errors

0a7b4b6

fabianvf force-pushed the ansible-test-kb-layout branch from 26a7661 to 0a7b4b6 Compare July 31, 2020 18:51

camilamacedo86 mentioned this pull request Aug 5, 2020

Fix undefined var in scaffolded molecule tests #3654

Merged

camilamacedo86 reviewed Aug 5, 2020

View reviewed changes

camilamacedo86 mentioned this pull request Oct 11, 2020

e2e tests: migrate the memcached molecule tests to use the new layout (1.0+) with mock samples stack #4011

Merged

camilamacedo86 mentioned this pull request Oct 21, 2020

Replace the test/ansible static files by a mock data generated with the samples stack #4025

Closed

camilamacedo86 reviewed Oct 30, 2020

View reviewed changes

camilamacedo86 closed this Oct 30, 2020

camilamacedo86 mentioned this pull request Dec 6, 2020

e2e-tests: fix advanced molecule ansible tests #4273

Merged

Conversation

asmacdo commented Jul 15, 2020

Uh oh!

Uh oh!

camilamacedo86 left a comment

Choose a reason for hiding this comment

Uh oh!

fabianvf commented Jul 15, 2020

Uh oh!

camilamacedo86 commented Jul 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joelanford commented Jul 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joelanford left a comment

Choose a reason for hiding this comment

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

Choose a reason for hiding this comment

Uh oh!

camilamacedo86 Jul 29, 2020 • 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!

camilamacedo86 left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ci-robot commented Jul 31, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

camilamacedo86 commented Oct 11, 2020

Uh oh!

camilamacedo86 left a comment • 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 commented Oct 30, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

camilamacedo86 commented Jul 15, 2020 •

edited

Loading

joelanford commented Jul 15, 2020 •

edited

Loading

camilamacedo86 Jul 29, 2020 •

edited

Loading

camilamacedo86 left a comment •

edited

Loading