Add helm plugin scaffolding new layout #3295

camilamacedo86 · 2020-06-25T19:05:30Z

Description of the change:
This PR adds the helm plugin scaffolding the files with the new layout and using the current sdk helm pkg implementation (image)

added e2e tests for helm new layout. See:/test/e2e-helm-new/e2e_suite.go and ./hack/tests/e2e-helm-new.sh
centralize the method ReplaceInFile in the test/internal/utils.go since it is useful for all tests
Impl e2e tests in shell for the new layout. See hack/tests/e2e-helm-new-shell.sh

Motivation

SDK is in a process to be integrated with KB which means that its project layouts will be aligned. More info : Integrating Kubebuilder and Operator SDK.

This PR adds the Helm Plugin which will provide the Helm project in the new layout by the command operator-sdk init --plugins=helm.operator-sdk.io/v1`. For a further understanding see the WIP: #329 and the doc Extensible CLI and Scaffolding Plugins.

Follow-ups

(Blocked by this one) - Fix OLM/CSV gen commands for Helm/Ansible operators - Done in Allows OLM subcommands work for Helm/Ansible new layout #3341
(Blocked by this one) Update docs to use plugins instead of the operator-sdk new --type and add api commands - Done in: docs: update helm docs for the new layout #3326
(Blocked by this one) Remove operator-sdk new --type=-helm removal: helm legacy scaffold #3343

cmd/operator-sdk/run/cmd.go

internal/plugins/helm/v1/init.go

internal/plugins/helm/v1/api.go

internal/plugins/helm/v1/scaffolds/templates/manager/config.go

**Description of the change:** Just moving the SDK scaffolds for plugins to the sdk dir and allow it to be re-used. **Motivation for the change:** The SDK scaffolds will be required for other plugins. See for example #3295

joelanford · 2020-07-14T03:25:16Z

hack/tests/e2e-helm-new-shell.sh

+        kubectl logs deployment.apps/nginx-operator-controller-manager -c manager --namespace=${test_namespace}
+        exit 1
+    fi
+


This test is missing the checks that curl the metrics endpoints that currently exists in the legacy shell-based test. We need to check all the same scenarios to make sure we don't accidentally break something in the transition.

hack/tests/e2e-helm-new-shell.sh

internal/plugins/helm/v1/chartutil/chart.go

internal/plugins/helm/v1/init.go

internal/plugins/helm/v1/scaffolds/api.go

internal/plugins/helm/v1/scaffolds/doc.go

internal/plugins/helm/v1/scaffolds/init.go

joelanford · 2020-07-14T03:53:37Z

test/e2e-helm-new/e2e_suite.go

+
+// Modified from https://github.com/kubernetes-sigs/kubebuilder/tree/39224f0/test/e2e/v3
+
+package e2e


This file isn't testing much more than what currently exists in the new shell based file.

Rather than adding two additional helm e2e tests (which would mean we'd have 3 total e2e tests just for helm!), I think for this PR we should stick with just the shell-based test.

Then in a follow-on, we can migrate the logic to this file and remove the shell-based tests.

internal/plugins/helm/v1/scaffolds/templates/makefile.go

joelanford · 2020-07-14T12:00:09Z

hack/lib/common.sh

+function setup_kustomize {
+  header_text "getting kustomize"
+  curl -s "https://raw.githubusercontent.com/kubernetes-sigs/kustomize/master/hack/install_kustomize.sh"  | bash
+  chmod u+x kustomize
+  mv kustomize "$1"/bin/kustomize
+}


Do we need this? I figured that since the make targets that require kustomize depend on it, and then the kustomize target would download it if it isn't already on the path.

Also it looks like the install_kustomize.sh script downloads the latest version of kustomize, whereas the Makefile has a hardcoded version. Shouldn't we be testing with the version that is scaffolded in the Makefile?

asmacdo · 2020-07-14T14:22:06Z

internal/plugins/helm/v1/scaffolds/api.go

@@ -0,0 +1,134 @@
+/*
+Copyright 2019 The Kubernetes Authors.
+Modifications copyright 2020 The Operator-SDK Authors


This looks deliberate, but I am surprised that this file wouldnt be a full Operator-SDK copyright file

The file is copied from KB and modified then, we need to respect the license. See: https://softwareengineering.stackexchange.com/questions/157968/how-to-manage-a-copyright-notice-in-an-open-source-project

In that case this should be:

Original work Copyright 2019 The Kubernetes Authors. Modified work Copyright 2020 The Operator-SDK Authors

Along with any other file that was copied with modifications.

joelanford · 2020-07-14T14:27:05Z

internal/plugins/helm/v1/scaffolds/templates/makefile.go

 	set -e ;\
 	mkdir -p bin ;\
-	curl -sSLo - https://github.com/kubernetes-sigs/kustomize/releases/download/kustomize/{{ .KustomizeVersion }}/kustomize_{{ .KustomizeVersion }}_${OS}_${ARCH}.tar.gz | tar xzf - -C bin/ ;\
+	echo https://github.com/kubernetes-sigs/kustomize/releases/download/kustomize/{{ .KustomizeVersion }}/kustomize_{{ .KustomizeVersion }}_$(OS)_$(ARCH).tar.gz ;\


Remove this echo line?

joelanford · 2020-07-14T14:35:02Z

internal/plugins/helm/v1/scaffolds/templates/makefile.go

-	if f.Image == "" {
-		f.Image = "controller:latest"
-	}


I see that we're explicitly setting f.Image in the scaffold calls, but with this defaulter removed, it is now possible to use templates.Makefile and end up with an invalid Makefile.

We either need to return an error if f.Image is not set OR we need to keep this as the default.

My preference would be to stay aligned with Kubebuilder's Go plugin, which leaves this conditional in so that f.Image is defaulted to controller:latest if it isn't explicitly set by a caller.

joelanford · 2020-07-14T14:37:39Z

hack/tests/e2e-helm.sh

 #!/usr/bin/env bash

-set -eux
+set -o errexit


Revert this file back to the legacy Helm e2e test.

In a follow-up (when the legacy Helm scaffolding is removed) we can rename e2e-helm-new-shell.sh to e2e-helm.sh. And in a separate follow-up, we can port the actual tests from Shell-based tests to Go-based tests and update this file accordingly.

joelanford · 2020-07-14T14:39:45Z

test/e2e-helm/e2e_suite.go

@@ -0,0 +1,157 @@
+// Copyright 2020 The Operator-SDK Authors


Remove this file for now. In a follow-up, we can port the shell-based test to a Go-based test and reintroduce this when it covers all of the existing test scenarios.

joelanford · 2020-07-14T14:40:07Z

test/e2e-helm/e2e_test.go

@@ -0,0 +1,29 @@
+// Copyright 2020 The Operator-SDK Authors


Ditto: https://github.com/operator-framework/operator-sdk/pull/3295/files#r454406994

camilamacedo86 · 2020-07-14T16:59:09Z

I am closing because I am testing in my fork. I will re-open soon.

estroz · 2020-07-14T17:50:56Z

test/internal/utils.go

 }
+
+// ReplaceInFile replaces all instances of old with new in the file at path.
+func ReplaceInFile(path, old, new string) {


Where else is this being used? If it isn't revert this back to being unexported in Go e2e tests.

I will revert it was added here to be used in both go tests. Now it will be part of a follow-up 👍

estroz · 2020-07-14T17:52:33Z

go.mod

 	github.com/Azure/go-autorest => github.com/Azure/go-autorest v13.3.2+incompatible // Required by OLM
 	github.com/mattn/go-sqlite3 => github.com/mattn/go-sqlite3 v1.10.0
 	k8s.io/client-go => k8s.io/client-go v0.18.2
+


Can you run make tidy before committing this?

make tidy do not remove the space. But I fixed that 👍 Tks.

estroz · 2020-07-14T17:53:25Z

hack/tests/e2e-helm-new.sh

@@ -0,0 +1,156 @@
+#!/usr/bin/env bash


IIRC we want to add all new e2e tests as ginkgo/gomega suites. Perhaps that is too much extra code for this PR which is already large, so at least do this in a follow-up if that's how we want to proceed with e2e tests.

Hi @estroz,

The first version was with the tests done in go like you wish 👍 . However, @joelanford asked for we keep here the same tests in the shell just for we ensure now that all is 100% then we can do a follow with the go tests as well.

camilamacedo86 · 2020-07-14T18:05:57Z

/reopen

openshift-ci-robot · 2020-07-14T18:05:58Z

@camilamacedo86: Failed to re-open PR: state cannot be changed. The helm-external-plugin branch was force-pushed or recreated.

In response to this:

/reopen

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

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 Jun 25, 2020

openshift-ci-robot requested review from hasbro17 and theishshah June 25, 2020 19:05

camilamacedo86 requested review from asmacdo, estroz and joelanford June 25, 2020 20:19

camilamacedo86 changed the title ~~WIP - add helm plugin using joes (not merge)~~ WIP - add helm plugin scaffolding new layout with legacy implementation Jun 27, 2020

camilamacedo86 added language/helm Issue is related to a Helm operator project kubebuilder-integration Relates to rewriting the SDK in Kubebuilder plugin form labels Jun 27, 2020

camilamacedo86 changed the title ~~WIP - add helm plugin scaffolding new layout with legacy implementation~~ Add helm plugin scaffolding new layout with legacy implementation Jul 1, 2020

camilamacedo86 marked this pull request as ready for review July 1, 2020 12: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 Jul 1, 2020

camilamacedo86 changed the title ~~Add helm plugin scaffolding new layout with legacy implementation~~ WIP: Add helm plugin scaffolding new layout with legacy implementation Jul 1, 2020

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

camilamacedo86 changed the title ~~WIP: Add helm plugin scaffolding new layout with legacy implementation~~ Add helm plugin scaffolding new layout with legacy implementation Jul 1, 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 1, 2020

camilamacedo86 closed this Jul 1, 2020

camilamacedo86 reopened this Jul 1, 2020

This was referenced Jul 1, 2020

docs: update helm docs for the new layout #3326

Merged

feat: allow know the project type by the config file #3330

Merged

camilamacedo86 commented Jul 1, 2020

View reviewed changes

cmd/operator-sdk/run/cmd.go Outdated Show resolved Hide resolved

camilamacedo86 changed the title ~~Add helm plugin scaffolding new layout with legacy implementation~~ WIP : Add helm plugin scaffolding new layout with legacy implementation Jul 2, 2020

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

camilamacedo86 mentioned this pull request Jul 2, 2020

feat: add kb scaffold utilities for plugins in internal/kubebuilder #3305

Merged

camilamacedo86 changed the title ~~WIP : Add helm plugin scaffolding new layout with legacy implementation~~ Add helm plugin scaffolding new layout with legacy implementation Jul 2, 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 2, 2020

This was referenced Jul 3, 2020

Allows OLM subcommands work for Helm/Ansible new layout #3341

Merged

doc: add migration guide for helm new layout #3342

Merged

removal: helm legacy scaffold #3343

Merged

camilamacedo86 added the blocked label Jul 3, 2020

camilamacedo86 mentioned this pull request Jul 8, 2020

Provide helm|ansible-operator binaries and replace run target scaffold for new layouts. #3355

Closed

estroz reviewed Jul 8, 2020

View reviewed changes

internal/plugins/helm/v1/init.go Show resolved Hide resolved

estroz reviewed Jul 8, 2020

View reviewed changes

internal/plugins/helm/v1/api.go Outdated Show resolved Hide resolved

estroz reviewed Jul 8, 2020

View reviewed changes

internal/plugins/helm/v1/api.go Outdated Show resolved Hide resolved

camilamacedo86 requested a review from estroz July 9, 2020 14:53

camilamacedo86 changed the title ~~Add helm plugin scaffolding new layout~~ WIP : Add helm plugin scaffolding new layout Jul 10, 2020

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

camilamacedo86 commented Jul 10, 2020

View reviewed changes

internal/plugins/helm/v1/scaffolds/templates/manager/config.go Show resolved Hide resolved

camilamacedo86 changed the title ~~WIP : Add helm plugin scaffolding new layout~~ Add helm plugin scaffolding new layout Jul 10, 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 10, 2020

camilamacedo86 removed the blocked label Jul 13, 2020

joelanford requested changes Jul 14, 2020

View reviewed changes

camilamacedo86 requested a review from joelanford July 14, 2020 11:08

joelanford reviewed Jul 14, 2020

View reviewed changes

asmacdo reviewed Jul 14, 2020

View reviewed changes

joelanford requested changes Jul 14, 2020

View reviewed changes

joelanford reviewed Jul 14, 2020

View reviewed changes

camilamacedo86 added 3 commits July 14, 2020 17:15

feat: add helm plugin scaffolding new layout

0219e24

test

1194ff9

test 2

93f1319

camilamacedo86 closed this Jul 14, 2020

estroz reviewed Jul 14, 2020

View reviewed changes

camilamacedo86 mentioned this pull request Jul 14, 2020

feat: add helm plugin scaffolding new layout #3421

Merged


		// Modified from https://github.com/kubernetes-sigs/kubebuilder/tree/39224f0/test/e2e/v3

		package e2e

Add helm plugin scaffolding new layout #3295

Add helm plugin scaffolding new layout #3295

Uh oh!

Conversation

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

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

camilamacedo86 Jul 14, 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!

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 commented Jul 14, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

camilamacedo86 Jul 14, 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!

estroz Jul 14, 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!

camilamacedo86 commented Jul 14, 2020

Uh oh!

openshift-ci-robot commented Jul 14, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

camilamacedo86 commented Jun 25, 2020 •

edited

Loading

camilamacedo86 Jul 14, 2020 •

edited

Loading

camilamacedo86 Jul 14, 2020 •

edited

Loading

estroz Jul 14, 2020 •

edited

Loading