Update make uninstall

[Boomatang]

closes: #3

• create and delete authorino-operator namespace (requied by make install-authorino)
• add make uninstall-authorino
• add make uninstall-cert-manager
• update make uninstall

Verification

• create a kind cluster
• list CRDS on cluster kubectl get crds
• install the authorino-operator resources make install
• check the CRDS again kubectl get crds
• run uninstall command make uninstall
• check the CRDS again kubectl get crds, this will equal the initial list of CRDS from the cluster.

[guicassolato]

make install does not install Authorino CRDs. I wonder if make uninstall should indeed try to undo what its counterpart did not do.

[Boomatang]

The make install does install the Authorino CRD, it is part of OPERATOR_MANIFESTS. The adding of the make uninstall-authorino is to mirror how the make install calls install-authorino. This change aligns closer with what its counterpart does.

[guicassolato]

My confusion. OPERATOR_MANIFESTS points to this file, which does not include Authorino CRDs. However, install depends on install-authorino. Makes sense.

[Boomatang]

I feel like I am missing something. The very first file in that manifest is the authorino CRD. What am I missing?

[guicassolato]

Authorino CRD belongs to the operator. AuthConfig CRD is the one that belongs to Authorino.

[Boomatang]

Okay I get you now when you said Authorio CRDs you meant Authorino as in Authorino the product and not the CRD kind. Happy that confusion was cleared up.
So to be sure, are you asking for anything to be changed here?

[guicassolato]

Deleting the operator's namespace seems a bit harsh. Effectively, this will decommission the operator instance. IWO, it's almost like a "undeploy" target.

[Boomatang]

Ya the namespace thing was something that kept biting me when doing this. As the create of the namespace is documented in the README I would be happy the remove the changes I made around the namespace.

[guicassolato]

I don't think we install cert-manager as part make install-authorino. We probably shouldn't uninstall it here either.

[Boomatang]

The make install-authorino does call install-cert-manager. This is aligning the install and uninstall targets.

[guicassolato]

I see that now.

Tricky one. Installing when it's already present causes less harm than uninstalling when you aren't supposed to. Not sure what best here.

[Boomatang]

Hmm, I see your point. The question I find my self asking is, what is the purpose of the make targets? Are they how we recommend the user installs the operator in production? If so how do we handle upgrades? Or is the purpose to aid developers in the day to day develop of the operator? In which case it would be better to revert all changes to a cluster? I am also not sure which is best but I do lean to the latter.

[guicassolato]

what is the purpose of the make targets?

Mainly dev and testing workflows, I'd say.

Are they how we recommend the user installs the operator in production?

No.

Or is the purpose to aid developers in the day to day develop of the operator?

Yes.

it would be better to revert all changes to a cluster? I am also not sure which is best but I do lean to the latter.

I was thinking... These make targets are useful for people who clone the repo, maybe they make changes, and want to try them out locally. They may no know exactly which YAMLs to apply for each step to of the workflow and therefore the targets provide them with a bit of guidance/documentation.

We cannot predict the exact scenario that development/test work is going on. E.g. maybe it's for a change in the controller's code, maybe it's a change in the Authorino CRD, maybe it's because of a change in Authorino itself that requires some tweak in the operator to be tested, maybe it's running in a fresh newly created cluster, maybe it's running on a preexisting cluster with preexisting stuff installed, maybe cert-manager is already running (installed for something else), maybe it's the first time the operator is installed, maybe it's a re-install, maybe it's to test a new version of CRDs on a preexisting install, etc.

With that in mind, perhaps a reasonable approach is providing very granular targets, for very specific operations, alongside with fewer more opinionated ones. For example,

• install: Installs Operator CRD/RBAC, plus Authorino CRD/RBAC

• install-operator: Installs Operator CRD/RBAC

• install-authorino: Installs Authorino CRD/RBAC

• install-cert-manager: Installs cert-manager

• uninstall: Uninstalls Operator CRD/RBAC, plus Authorino CRD/RBAC

• uninstall-operator: Uninstalls Operator CRD/RBAC

• uninstall-authorino: Uninstalls Authorino CRD/RBAC

• uninstall-cert-manager: Uninstalls cert-manager

• namespace: Creates authorino-operator namespace

• delete-namespace: Deletes authorino-operator namespace

• deploy: Creates Deployments of the Operator and webhooks; depends on install, cert-manager, namespace

• deploy-webhooks: Creates Deployment of the webhook; depends on install-authorino, cert-manager, namespace

• undeploy: Deletes Deployments of the Operator and webhooks

• undeploy-webhooks: Deletes Deployment of the webhook

• run: Runs the operator locally (with go run); depends on install

I guess the pattern being:

• <action> = runs deps + action itself; safe to run if deps already present
• un<action> = undo the action only

WDYT?

[Boomatang]

Maybe, it is this idea of easy of use. The idea that <action> installs the required dependencies makes for a very easy of use. But not having the same easy of use for the un<action> seems wrong or at least creates a double standard. At the same time that issue of undoing something that was existing pre <action> is far worse.

While I think this is a compromise and contradiction in usability, it is also possible the correct solution for now. Change would need some docs update also and the scope of some current targets would need to be reduced. The example off the top of my head is how the install target in a nest way also calls install-cert-manager. Which I think is not what your expecting to happen.

[guicassolato]

LGTM. Thanks, @Boomatang!