WIP: [not merge] - just todo's and questions

.golangci.yml

@@ -1,16 +1,44 @@
+# todo: we need to add the following for we ensure that all exported methods from the lib will be doc
+# E.g exported method `PostHookFunc.Exec` should have comment or be unexported (golint)
+issues:
+  # don't skip warning about doc comments
+  exclude-use-default: false
+
+  # restore some of the defaults
+  # (fill in the rest as needed)
+  exclude-rules:
+    - linters: [errcheck]
+      text: "Error return value of .((os\\.)?std(out|err)\\..*|.*Close|.*Flush|os\\.Remove(All)?|.*printf?|os\\.(Un)?Setenv). is not checked"
+# todo(camilamacedo86): missing checks  - IMO we need to apply here the same ones which are enable for kb
 linters:
   disable-all: true
   enable:
-    - govet
+    - deadcode
+    - dupl # need to fix issues found with this check
     - errcheck
-    - staticcheck
-    - unused
+    - goconst # need to fix issues found with this check
+    - gocyclo
+    - gofmt
+    - goimports
+    - golint
+    - gosec
     - gosimple
-    - structcheck
-    - varcheck
+    - govet
     - ineffassign
-    - deadcode
+    - interfacer # need to fix issues found with this check
+    - lll # need to fix issues found with this check
+    - maligned # need to fix issues found with this check
+    - misspell
+    - nakedret
+    - prealloc
+    - scopelint # need to fix issues found with this check
+    - staticcheck
+    - structcheck
     - typecheck
+    - unconvert
+    - unparam
+    - unused
+    - varcheck
 
 run:
-  deadline: 5m
+  deadline: 5m

.travis.yml

@@ -8,3 +8,5 @@ before_install:
 script:
 - make test manager lint && git diff
 - $GOPATH/bin/goveralls -service=travis-ci -coverprofile=cover.out
+
+#todo: if we will have the scaffolds here as well then we need impl the testdata gen and an e2e test for that.

Dockerfile

@@ -22,5 +22,6 @@ FROM gcr.io/distroless/static:nonroot
 WORKDIR /
 COPY --from=builder /workspace/manager .
 USER nonroot:nonroot
-
+# todo: should we not do here the same setup valid for OCP (user_setup)
+# and how works here the `exec ${OPERATOR} exec-entrypoint helm --watches-file=$HOME/watches.yaml $@`
 ENTRYPOINT ["/manager"]

Makefile

@@ -1,8 +1,7 @@
-
 # Image URL to use all building/pushing image targets
 IMG ?= quay.io/joelanford/helm-operator
 
-SHELL=/bin/bash
+SHELL=/bin/bash  # todo(camilamacedo86): why it is required?
 # Get the currently used golang install path (in GOPATH/bin, unless GOBIN is set)
 ifeq (,$(shell go env GOBIN))
 GOBIN=$(shell go env GOPATH)/bin
@@ -14,6 +13,7 @@ all: manager
 
 # Run tests
 test: generate fmt vet testbin
+	# todo(camilamacedo86): why `-race -covermode atomic`?
 	go test -race -covermode atomic -coverprofile cover.out ./...
 
 # Build manager binary
@@ -31,6 +31,10 @@ vet:
 lint: golangci-lint
 	$(GOLANGCI_LINT) run
 
+# todo(camilamacedo86): do not work
+# $ make generate
+  #/Users/camilamacedo/go/bin/controller-gen object:headerFile=./hack/boilerplate.go.txt paths="./..."
+  #open ./hack/boilerplate.go.txt: no such file or directory
 # Generate code
 generate: controller-gen
 	$(CONTROLLER_GEN) object:headerFile=./hack/boilerplate.go.txt paths="./..."
@@ -43,6 +47,9 @@ docker-build: test
 docker-push:
 	docker push ${IMG}
 
+
+# todo(camilamacedo86): could we not have it just in the travis?
+# IMO: we should have just a target to run the lint with the setup .golangci.yml and we should not download it. Just in the travis.
 # find or download controller-gen
 # download controller-gen if necessary
 golangci-lint:
@@ -84,3 +91,14 @@ testbin:
 	mkdir -p testbin
 	[[ -x testbin/etcd ]] || curl -L https://storage.googleapis.com/etcd/${ETCD_VER}/etcd-${ETCD_VER}-${OS}-${ARCH}.tar.gz | tar zx -C testbin --strip-components=1 etcd-${ETCD_VER}-${OS}-${ARCH}/etcd
 	[[ -x testbin/kube-apiserver && -x testbin/kubectl ]] || curl -L https://dl.k8s.io/${K8S_VER}/kubernetes-server-${OS}-${ARCH}.tar.gz | tar zx -C testbin --strip-components=3 kubernetes/server/bin/kube-apiserver kubernetes/server/bin/kubectl
+
+# $ make testbin
+#  mkdir -p testbin
+#  [[ -x testbin/etcd ]] || curl -L https://storage.googleapis.com/etcd/v3.4.3/etcd-v3.4.3-darwin-amd64.tar.gz | tar zx -C testbin --strip-components=1 etcd-v3.4.3-darwin-amd64/etcd
+#    % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
+#                                   Dload  Upload   Total   Spent    Left  Speed
+#  100   205  100   205    0     0    510      0 --:--:-- --:--:-- --:--:--   511
+#  tar: Unrecognized archive format
+#  tar: etcd-v3.4.3-darwin-amd64/etcd: Not found in archive
+#  tar: Error exit delayed from previous errors.
+#  make: *** [Makefile:118: testbin] Error 1

example/Dockerfile

@@ -1,3 +1,4 @@
+# only the makefile and dockerfile  are the only scaffods that need to be updated from the base
 FROM quay.io/joelanford/helm-operator:latest
 
 ENV HOME=/opt/helm

example/Makefile

@@ -1,30 +1,49 @@
+# It will be == kb / new layout less the targets to work with GO
+# see that we cannot add the generate (ansible/helm) will not gen code
+# go test, vet fmt ^
 
 # Image URL to use all building/pushing image targets
-IMG ?= quay.io/joelanford/nginx-operator
+IMG ?= controller:latest
+# Produce CRDs that work back to Kubernetes 1.11 (no version conversion)
+CRD_OPTIONS ?= "crd:trivialVersions=true"
 
-all: docker-build
+# Get the currently used golang install path (in GOPATH/bin, unless GOBIN is set)
+ifeq (,$(shell go env GOBIN))
+GOBIN=$(shell go env GOPATH)/bin
+else
+GOBIN=$(shell go env GOBIN)
+endif
+
+all: manager
+
+# Build manager binary
+manager: manifests docker-build
 
 # Run against the configured Kubernetes cluster in ~/.kube/config
-run:
+run: manifests
 	go run ../main.go
 
 # Install CRDs into a cluster
-install:
-	kustomize build config/crd | kubectl apply -f -
+install: manifests kustomize
+	$(KUSTOMIZE) build config/crd | kubectl apply -f -
 
 # Uninstall CRDs from a cluster
-uninstall:
-	kustomize build config/crd | kubectl delete -f -
+uninstall: manifests kustomize
+	$(KUSTOMIZE) build config/crd | kubectl delete -f -
 
 # Deploy controller in the configured Kubernetes cluster in ~/.kube/config
-deploy:
-	cd config/manager && kustomize edit set image controller=${IMG}
-	kustomize build config/default | kubectl apply -f -
+deploy: manifests kustomize
+	cd config/manager && $(KUSTOMIZE) edit set image controller=${IMG}
+	$(KUSTOMIZE) build config/default | kubectl apply -f -
 
+# It is something that we should push to upstream since is valid for all scenarios
 # Undeploy controller in the configured Kubernetes cluster in ~/.kube/config
 undeploy:
-	cd config/manager && kustomize edit set image controller=${IMG}
-	kustomize build config/default | kubectl delete -f -
+	cd config/manager && $(KUSTOMIZE) edit set image controller=${IMG}
+	$(KUSTOMIZE) build config/default | kubectl delete -f -
+
+manifests:
+	$(CONTROLLER_GEN) $(CRD_OPTIONS) rbac:roleName=manager-role webhook paths="./..." output:crd:artifacts:config=config/crd/bases
 
 # Build the docker image
 docker-build:
@@ -33,3 +52,35 @@ docker-build:
 # Push the docker image
 docker-push:
 	docker push ${IMG}
+
+# find or download controller-gen
+# download controller-gen if necessary
+controller-gen:
+ifeq (, $(shell which controller-gen))
+	@{ \
+	set -e ;\
+	CONTROLLER_GEN_TMP_DIR=$$(mktemp -d) ;\
+	cd $$CONTROLLER_GEN_TMP_DIR ;\
+	go mod init tmp ;\
+	go get sigs.k8s.io/controller-tools/cmd/[email protected] ;\
+	rm -rf $$CONTROLLER_GEN_TMP_DIR ;\
+	}
+CONTROLLER_GEN=$(GOBIN)/controller-gen
+else
+CONTROLLER_GEN=$(shell which controller-gen)
+endif
+
+kustomize:
+ifeq (, $(shell which kustomize))
+	@{ \
+	set -e ;\
+	KUSTOMIZE_GEN_TMP_DIR=$$(mktemp -d) ;\
+	cd $$KUSTOMIZE_GEN_TMP_DIR ;\
+	go mod init tmp ;\
+	go get sigs.k8s.io/kustomize/kustomize/[email protected] ;\
+	rm -rf $$KUSTOMIZE_GEN_TMP_DIR ;\
+	}
+KUSTOMIZE=$(GOBIN)/kustomize
+else
+KUSTOMIZE=$(shell which kustomize)
+endif

go.mod

@@ -24,6 +24,7 @@ require (
 	sigs.k8s.io/yaml v1.2.0
 )
 
+// Why we are keeping the following comments here?
 //replace (
 //	// github.com/Azure/go-autorest/autorest has different versions for the Go
 //	// modules than it does for releases on the repository. Note the correct

main.go

@@ -23,7 +23,7 @@ import (
 	"time"
 
 	"github.com/spf13/pflag"
-	"go.uber.org/zap"
+	"go.uber.org/zap" // todo(camilamacedo86): why we are adding this lib? Shuld we not keep it == kube?
 	_ "k8s.io/client-go/plugin/pkg/client/auth/gcp"
 	"k8s.io/klog"
 	ctrl "sigs.k8s.io/controller-runtime"
@@ -45,9 +45,11 @@ func printVersion() {
 		"go", runtime.Version(),
 		"GOOS", runtime.GOOS,
 		"GOARCH", runtime.GOARCH,
-		"helm-operator", version.Version)
+		"helm-operator", version.Version) // Should not we inform the helm plugin version here? So, should it not be part of SDK impl and no lib?
 }
 
+// The implementation here is equivalent what we have in the
+// /operator-sdk/pkg/helm/run.go internal/scaffold/helm/main.go and helm flags
 func main() {
 	var (
 		metricsAddr             string
@@ -62,10 +64,12 @@ func main() {
 		// Deprecated: use defaultMaxConcurrentReconciles
 		defaultMaxWorkers int
 	)
-
+	//todo(camilamacedo86): why the port is not 8080?
 	pflag.StringVar(&metricsAddr, "metrics-addr", "0.0.0.0:8383", "The address the metric endpoint binds to.")
 	pflag.BoolVar(&enableLeaderElection, "enable-leader-election", false,
 		"Enable leader election for controller manager. Enabling this will ensure there is only one active controller manager.")
+
+	//todo(camilamacedo86): the following ones has not in the kube. Wy we need the leaderElectionID and leaderElectionNamespace?
 	pflag.StringVar(&leaderElectionID, "leader-election-id", "",
 		"Name of the configmap that is used for holding the leader lock.")
 	pflag.StringVar(&leaderElectionNamespace, "leader-election-namespace", "",
@@ -95,7 +99,7 @@ func main() {
 	))
 
 	printVersion()
-
+	// todo: Should we not to do this deprecations now in SDK (ASAP) and then may we can just remove from here?
 	// Deprecated: --max-workers flag does not align well with the name of the option it configures on the controller
 	//   (MaxConcurrentReconciles). Flag `--max-concurrent-reconciles` should be used instead.
 	if pflag.Lookup("max-workers").Changed {
@@ -106,7 +110,7 @@ func main() {
 			defaultMaxConcurrentReconciles = defaultMaxWorkers
 		}
 	}
-
+	// Why we need a flag for `--leader-election-id`? What is the user case that would be required pass this value?
 	// Deprecated: OPERATOR_NAME environment variable is an artifact of the legacy operator-sdk project scaffolding.
 	//   Flag `--leader-election-id` should be used instead.
 	if operatorName, found := os.LookupEnv("OPERATOR_NAME"); found {
@@ -117,12 +121,13 @@ func main() {
 			leaderElectionID = operatorName
 		}
 	}
-
 	options := ctrl.Options{
-		MetricsBindAddress:      "0.0.0.0:8383",
-		LeaderElection:          enableLeaderElection,
+		MetricsBindAddress: metricsAddr,
+		LeaderElection:     enableLeaderElection,
+		// todo: add:
+		Port:                    9443, // should not it be == kubebuilder?
 		LeaderElectionID:        leaderElectionID,
-		LeaderElectionNamespace: leaderElectionNamespace,
+		LeaderElectionNamespace: leaderElectionNamespace, // todo: why we need the ns?
 		NewClient:               manager.NewDelegatingClientFunc(),
 	}
 	manager.ConfigureWatchNamespaces(&options, setupLog)
@@ -137,7 +142,9 @@ func main() {
 		setupLog.Error(err, "unable to load watches.yaml", "path", watchesFile)
 		os.Exit(1)
 	}
-
+	// Not for now. Just a thought for the future: Could we not work with markers to inject
+	// the code based on the helm/ansible charts instead which would allow users
+	// also add Go code. Could not it be a solution for the hibrid ones?
 	for _, w := range ws {
 		reconcilePeriod := defaultReconcilePeriod
 		if w.ReconcilePeriod != nil {
@@ -174,6 +181,17 @@ func main() {
 
 	// TODO(joelanford): kube-state-metrics?
 
+	// Are we sure that the "https://godoc.org/sigs.k8s.io/controller-runtime/pkg/manager#LeaderElectionRunnable"
+	// is able to do all that is done with:
+	// Become the leader before proceeding
+	//	err = leader.Become(ctx, operatorName+"-lock")
+	//	if err != nil {
+	//		log.Error(err, "Failed to become leader.")
+	//		return err
+	//	}
+	// Should we check with we would need to keep the leader.Become or try to push something to upstream?
+	// Why we still using it in the legacy projects?
+
 	setupLog.Info("starting manager")
 	if err := mgr.Start(ctrl.SetupSignalHandler()); err != nil {
 		setupLog.Error(err, "problem running manager")

pkg/annotation/annotation.go

@@ -52,11 +52,14 @@ type InstallDisableHooks struct {
 var _ Install = &InstallDisableHooks{}
 
 const (
-	DefaultDomain                    = "helm.operator-sdk"
+	DefaultDomain = "helm.operator-sdk"
+	// The following ones shows a new behaviour introduced as well. todo: understand user case
 	DefaultInstallDisableHooksName   = DefaultDomain + "/install-disable-hooks"
 	DefaultUpgradeDisableHooksName   = DefaultDomain + "/upgrade-disable-hooks"
 	DefaultUninstallDisableHooksName = DefaultDomain + "/uninstall-disable-hooks"
 
+	// we have the annotation here, however where is the  release.ForceUpdate?
+	// shows that we are not forcing the upgrade when it is informed
 	DefaultUpgradeForceName = DefaultDomain + "/upgrade-force"
 
 	DefaultInstallDescriptionName   = DefaultDomain + "/install-description"

pkg/client/actionclient.go

@@ -151,7 +151,7 @@ func (c *actionClient) Upgrade(name, namespace string, chrt *chart.Chart, vals m
 	upgrade := action.NewUpgrade(c.conf)
 	upgrade.PostRenderer = c.postRenderer
 	for _, o := range opts {
-		if err := o(upgrade); err != nil {
+		if err := o(upgrade); err != nil { // todo: check I did not get it.
 			return nil, err
 		}
 	}

pkg/internal/sdk/fake/controller.go

@@ -13,7 +13,7 @@ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 See the License for the specific language governing permissions and
 limitations under the License.
 */
-
+// would not make sense move it to testutils?
 package fake
 
 import (

pkg/internal/sdk/handler/enqueue_annotation.go

@@ -13,7 +13,8 @@ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 See the License for the specific language governing permissions and
 limitations under the License.
 */
-
+// I think would be nice we are able to do : https://github.com/operator-framework/operator-sdk/issues/2962
+// However, it is part of SDK lib
 package handler
 
 import (