Update Makefile targets for consistency #227

tmshort · 2023-05-22T14:53:05Z

Description

Update Makefile targets for consistency

Fixes #226

Rename the install target to kind-deploy
Remove uninstall target
Rename the kind-cluster-cleanup target to kind-clean
Remove the generate target from kind-deploy
Swap e2e/test-e2e targets
Update help descriptions
Add help-extended,
Change e2e workflow to use test-e2e

Reviewer Checklist

API Go Documentation
Tests: Unit Tests (and E2E Tests, if appropriate)
Comprehensive Commit Messages
Links to related GitHub Issue(s)

varshaprasad96 · 2023-05-22T16:19:42Z

Makefile

+kind-deploy: kind manifests kustomize ## Install controller and dependencies onto the kind cluster
+	kubectl kustomize config/default > operator-controller.yaml
+	envsubst '$$CATALOGD_VERSION,$$CERT_MGR_VERSION,$$RUKPAK_VERSION,$$MANIFEST' < scripts/install.tpl.sh | bash -s


Just curious, if we have this, then do we need deploy target. I see that make kind-deploy would controller and the dependencies and the latter would only install the controller, but is there a use case where we would like to support just the installation of the controller with (probably) some other version of residual dependencies running on cluster.

Also, thoughts on having the naming as kind-deploy-e2e? Just so that it's easier to interpret the difference between the two.

I could definitely see a case for separate behaviors.

make deploy would potentially be used in environments where the other components are already installed:

make -C rukpak deploy make -C catalogd deploy make -c operator-controller deploy

kind-deploy is used for both e2e and run targets, so I'd prefer to keep it as kind-deploy.

TBH, I'm thinking that a number of these targets don't really need to be as exposed (via make help) as they currently are.

How much of this is theoretical? I'd very much like to limit our Makefile to having as few supported use-cases as possible, for the sake of our own sanity.

I personally use (pretty much exclusively):

make build to build the binaries locally.

make run to rebuild/redeploy everything in a fresh kind cluster

make test-unit (or whatever we call it) to run the unit tests

make test-e2e to run the e2e tests

make verify to make sure I've got all the auto-generated stuff updated.

Would it be worthwhile to do a quick contributor survey to see what people actually use and then go from there?

https://kubernetes.slack.com/archives/C0181L6JYQ2/p1685127976360479

joelanford · 2023-06-01T13:36:53Z

.github/workflows/deploy.yaml

+        make kind-cluster
+        make install
+        kubectl get crds operators.operators.operatorframework.io
+        make uninstall
+        ! kubectl get crds operators.operators.operatorframework.io
+        make deploy
+        kubectl get ns operator-controller-system
+        make undeploy
+        ! kubectl get ns operator-controller-system


These steps essentially happen during the course of running the e2e, don't they? I suppose we don't try uninstall and undeploy during the e2e, but maybe we could?

joelanford · 2023-06-01T13:40:06Z

Makefile

@@ -133,24 +146,26 @@ release: goreleaser ## Runs goreleaser for the operator-controller. By default,
 	$(GORELEASER) $(GORELEASER_ARGS)

 quickstart: export MANIFEST="https://github.com/operator-framework/operator-controller/releases/download/$(VERSION)/operator-controller.yaml"
-quickstart: kustomize generate ## Generate the installation release manifests and scripts
+quickstart: kustomize ### Generate the installation release manifests and scripts with dependencies


Curious why we're removing generate as a dependency. Without it, the generated operator-controller.yaml may be out-dated.

Nevermind, I see generate only generates go code. But perhaps manifests should be a dependency of quickstart?

joelanford · 2023-06-01T13:47:30Z

Makefile

Can't comment on the line since it is unchanged, but I'd propose moving release to extended help.

joelanford · 2023-06-01T13:49:43Z

Makefile

-install: manifests kustomize generate ## Install CRDs into the K8s cluster specified in ~/.kube/config.
-	kubectl kustomize config/default > operator-controller.yaml
-	envsubst '$$CATALOGD_VERSION,$$CERT_MGR_VERSION,$$RUKPAK_VERSION,$$MANIFEST' < scripts/install.tpl.sh | bash -s
+install: manifests kustomize ### Install CRDs into the K8s cluster specified in ~/.kube/config.


Are install/uninstall valuable to anyone? Or is everyone always going to use deploy/undeploy? Wondering if we can drop install and uninstall.

install just does the CRDs... which aren't of much use without the controller. There are some processes that install CRDs first, then everything else... but operator-controller kinda needs to be first anyways.

There are some processes that install CRDs first

Our own processes, or theoretical third party processes? If the latter, and only the latter, I'm still of the opinion that we stick with just the deploy/undeploy targets.

Third-party; it's how we tried to service-mesh at my last job.

tmshort · 2023-06-01T18:32:10Z

ping?

everettraven

A nit in the action but other than that the changes look reasonable to me

everettraven · 2023-06-01T19:39:43Z

.github/workflows/deploy.yaml

+      with:
+        go-version-file: go.mod
+
+    - name: Run basic unit tests


calling this a "unit" test would be confusing to me if all I saw was the name of the step and not what was actually happening

joelanford · 2023-06-01T21:30:49Z

.github/workflows/deploy.yaml

+        make kind-cluster
+        make deploy
+        kubectl get crds operators.operators.operatorframework.io
+        kubectl get ns operator-controller-system
+        make undeploy
+        ! kubectl get ns operator-controller-system
+        ! kubectl get crds operators.operators.operatorframework.io


Two things:

nit: it looks like there's a mix of spaces and tabs here for indentation (unless GH UI is tricking me). Change tabs to spaces (only because that's how the rest of the file looks, not trying to start a holy war 😛 )?

My last comment I think was eaten by a new commit, repeating here: These steps essentially happen during the course of running the e2e, don't they? I suppose we don't try uninstall and undeploy during the e2e, but maybe we could?

Since removing install/uninstall, I put in undeploy

There are no tabs... so GH UI must be tricking you?

codecov · 2023-08-10T16:09:17Z

Codecov Report

Merging #227 (74d8148) into main (2114da6) will increase coverage by 0.21%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #227      +/-   ##
==========================================
+ Coverage   81.46%   81.68%   +0.21%     
==========================================
  Files          21       21              
  Lines         928      928              
==========================================
+ Hits          756      758       +2     
+ Misses        119      118       -1     
+ Partials       53       52       -1

Flag	Coverage Δ
e2e	`62.28% <ø> (+0.21%)`	⬆️
unit	`77.22% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

see 1 file with indirect coverage changes

grokspawn · 2023-08-16T14:59:18Z

I embrace the thesis of this PR, but IMHO the difference between two-hashes and three-hashes is too subtle to avoid confusion.

I believe the file separator for awk is arbitrary. Could we change to something like #extended or something which more intuitively indicates the distinction from ##?

tmshort · 2023-08-16T19:54:29Z

I believe the file separator for awk is arbitrary. Could we change to something like #extended or something which more intuitively indicates the distinction from ##?

#HELP
#EXHELP
#SECTION

??

Fixes operator-framework#226 * Rename the `install` target to `kind-deploy` * Remove `uninstall` target * Rename the `kind-cluster-cleanup` target to `kind-clean` * Remove the `generate` target from `kind-deploy` * Swap `e2e`/`test-e2e` targets * Update help descriptions * Add `help-extended`, * Change e2e workflow to use `test-e2e` Signed-off-by: Todd Short <tshort@redhat.com>

grokspawn · 2023-08-25T15:29:28Z

/approve
/lgtm

tmshort force-pushed the update-makefile branch from 86b8f2a to f58f593 Compare May 22, 2023 15:01

varshaprasad96 reviewed May 22, 2023

View reviewed changes

tmshort force-pushed the update-makefile branch from f58f593 to a1d118a Compare May 30, 2023 14:00

joelanford reviewed Jun 1, 2023

View reviewed changes

tmshort force-pushed the update-makefile branch from bea89e4 to a46e2af Compare June 1, 2023 13:42

joelanford reviewed Jun 1, 2023

View reviewed changes

tmshort force-pushed the update-makefile branch 2 times, most recently from d134286 to 1887bd6 Compare June 1, 2023 14:43

everettraven approved these changes Jun 1, 2023

View reviewed changes

joelanford reviewed Jun 1, 2023

View reviewed changes

tmshort force-pushed the update-makefile branch from 1887bd6 to 63df498 Compare June 5, 2023 18:00

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 8, 2023

tmshort force-pushed the update-makefile branch from 63df498 to 50e2a9e Compare August 10, 2023 16:03

tmshort requested a review from a team as a code owner August 10, 2023 16:03

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 10, 2023

tmshort force-pushed the update-makefile branch from 50e2a9e to 5155964 Compare August 16, 2023 19:53

tmshort force-pushed the update-makefile branch from 5155964 to 56d79f9 Compare August 16, 2023 20:06

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 24, 2023

tmshort force-pushed the update-makefile branch from 56d79f9 to 74d8148 Compare August 25, 2023 14:19

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 25, 2023

openshift-ci bot assigned grokspawn Aug 25, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 25, 2023

grokspawn approved these changes Aug 25, 2023

View reviewed changes

tmshort added this pull request to the merge queue Sep 1, 2023

Merged via the queue into operator-framework:main with commit 87d2087 Sep 1, 2023
11 checks passed

tmshort deleted the update-makefile branch September 5, 2023 17:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update Makefile targets for consistency #227

Update Makefile targets for consistency #227

tmshort commented May 22, 2023 •

edited

Loading

varshaprasad96 May 22, 2023

tmshort May 22, 2023

joelanford May 26, 2023 •

edited

Loading

tmshort May 26, 2023

joelanford Jun 1, 2023

joelanford Jun 1, 2023

joelanford Jun 1, 2023

joelanford Jun 1, 2023

joelanford Jun 1, 2023

tmshort Jun 1, 2023

joelanford Jun 1, 2023

tmshort Jun 1, 2023

tmshort commented Jun 1, 2023

everettraven left a comment

everettraven Jun 1, 2023

joelanford Jun 1, 2023

tmshort Jun 5, 2023

tmshort Jun 5, 2023

codecov bot commented Aug 10, 2023 •

edited

Loading

grokspawn commented Aug 16, 2023 •

edited

Loading

tmshort commented Aug 16, 2023 •

edited

Loading

grokspawn commented Aug 25, 2023

Update Makefile targets for consistency #227

Update Makefile targets for consistency #227

Conversation

tmshort commented May 22, 2023 • edited Loading

Description

Reviewer Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joelanford May 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmshort commented Jun 1, 2023

everettraven left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Aug 10, 2023 • edited Loading

Codecov Report

grokspawn commented Aug 16, 2023 • edited Loading

tmshort commented Aug 16, 2023 • edited Loading

grokspawn commented Aug 25, 2023

tmshort commented May 22, 2023 •

edited

Loading

joelanford May 26, 2023 •

edited

Loading

codecov bot commented Aug 10, 2023 •

edited

Loading

grokspawn commented Aug 16, 2023 •

edited

Loading

tmshort commented Aug 16, 2023 •

edited

Loading