Skip to content

Add targets to Makefile #120

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed

Conversation

varshaprasad96
Copy link
Member

This commit adds two additional Makefile targets to Makefile:

cleanup: Since operator-controller is the single point of entry and make run installs Rukpak and Cert-manager, it would be good to have a target that cleans up the cluster of controller and all its dependencies.

run-local: To be able to run the controller locally, it is useful for developments so that we need not always have to build image to test operator. This is similar to make run target in SDK generated projects (https://github.com/operator-framework/operator-sdk/blob/5cbdad9209332043b7c730856b6302edc8996faf/testdata/go/v3/memcached-operator/Makefile#L118)

@varshaprasad96
Copy link
Member Author

cc: @dtfranz @tmshort could you please have a look.

@@ -100,6 +100,10 @@ build: manifests generate fmt vet ## Build manager binary.
.PHONY: run
run: docker-build kind-cluster kind-load cert-mgr rukpak install deploy wait ## Build the operator-controller then deploy it into a new kind cluster.

.PHONY: run-local
run-local: manifests generate fmt vet ## Run a controller from your host. Make sure that necessary CRDs are installed in the cluster.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to do an install here, so that CRDs are install on the default cluster?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking about it, then realized that we need rukpak and cert-manager CRDs too. Which I why I excluded everything instead of selectively installing only operator CRDs.

Makefile Outdated
Comment on lines 175 to 177
cleanup: undeploy uninstall
kubectl delete --ignore-not-found=$(ignore-not-found) -f https://github.com/cert-manager/cert-manager/releases/download/$(CERT_MGR_VERSION)/cert-manager.yaml
kubectl delete --ignore-not-found=$(ignore-not-found) -f https://github.com/operator-framework/rukpak/releases/latest/download/rukpak.yaml
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wondering if these should be targets instead, and rename the rukpak and cert-mgr targets?
e.g.
rukpak -> install-rukpak
cert-mgr -> install-cert-manager

Then, add uninstall-rukpak and uninstall-cert-mgr recipes and then as dependencies to cleanup?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good idea! Thoughts on bundling cert-manager and rukpak together in a single install and uninstall target?
Because cert-manager is a requirement for Rukpak, we won't have to install cert-manager alone ever (I guess).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably keep them separate, fine-grained control is more flexible.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for cleanup, what about just deleting the kind cluster?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a use case for actually deleting individual dependencies or subcomponents? If not, my preference is to do the simplest/smallest thing that devs will actually use. Whenever I contribute to an unfamiliar project, I'm pretty put off when there are tons and tons of make targets because its never clear what is useful to me and what isn't without building a full mental picture of the whole thing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I guess I'm okay with a bunch of targets if we make sure its apparent which of them are the primary ones we expect contributors to use. Once they're comfortable with those, then they can dig more into the Makefile and use bits and pieces of the secondary targets to fine-tune their workflows.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joelanford @tmshort Got it, I agree that if we are if we are considering this target to do the opposite of make run, then it should tear the cluster down.

Though as a developer (IMO), I feel its still useful to have an option to entirely remove the installed olm components instead of forcefully shutting the cluster down and creating a new one every time. This is just for faster cycles of development, where I don't care of the existing polluted cluster state (unless it interferes causing issues) - (though spinning up a cluster doesn't take much effort, I would avoid doing it repeatedly for every small cycle of development).
But as it may not be valuable to make it as a separate Makefile target and can end up causing more confusion, I'll modify the PR to just tear down the kind cluster.

Copy link
Member

@joelanford joelanford Feb 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's what I was getting at with my question @varshaprasad96. Would it make sense to have a target that:

  • build the images
  • loads them into kind
  • generate CRDs/rbac
  • kubectl delete --ignore-not-present the manifest
  • kubectl apply the manifest

Then make run becomes:

  1. setup the kind cluster
  2. Install rukpak + cert manager
  3. do the above

But then the dev cycle could just run step 3 only.

Copy link
Member Author

@varshaprasad96 varshaprasad96 Feb 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So something like this:

install-rukpak:

install-certmgr:

uninstall-rukpak:

uninstall-certmgr:

run-image:
	docker-build kind-load install deploy

run: 
    kind-cluster run-image 

clean:
	kind delete <cluster>

run-local: manifests generate fmt vet
         go run ./main.go

Use cases:

For developer, they could:

  1. For running it in an existing cluster: make run-image
  2. For running operator-controller locally: make install-certmgr,make install-rukpak,make install,make run-local

For developer who were running operator controller locally or would like to clean existing cluster:

  • make uninstall make undeploy make uninstall-rukpak make uninstall-certmgr

For a someone trying to run OLM in a single shot:

  • make run - does everything for you.
  • make clean - tear down the cluster

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As @tmshort suggested, we could bundle up make run-local to be:

run-local: manifests generate fmt vet install-certmgr install-rukpak install
         go run ./main.go

to make it easier for devs, or leave it as is so that they are aware of what components are being installed (that way its transparent on what to uninstall). Its a tradeoff, I'm fine either way

@awgreene
Copy link
Member

awgreene commented Feb 9, 2023

Closing and reopening this to test that DCO is enabled.

@awgreene awgreene closed this Feb 9, 2023
@awgreene awgreene reopened this Feb 9, 2023
@perdasilva
Copy link
Contributor

@varshaprasad96 I think these changes are really nice, could I ask you to please rebase?

This commit adds two additional Makefile targets to Makefile:

- Separate targets to install and uninstall - Rukpak and cert-mgr.

- cleanup: Since operator-controller is the single point of entry and `make run` installs
	 Rukpak and Cert-manager, it would be good to have a target that cleans up the
	 cluster of controller and all its dependencies.

- run-local: To be able to run the controller locally, it is useful for developments so that
	   we need not always have to build image to test operator.

Signed-off-by: Varsha Prasad Narsing <[email protected]>
Signed-off-by: Varsha Prasad Narsing <[email protected]>
@varshaprasad96
Copy link
Member Author

Modified and rebased the PR to address changes according to #120 (comment)

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 13, 2023
@openshift-merge-robot
Copy link

PR needs rebase.

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.

@tmshort
Copy link
Contributor

tmshort commented Apr 19, 2023

Rebase needed again

@tmshort
Copy link
Contributor

tmshort commented Sep 6, 2023

@varshaprasad96 this needs a rebase. Do you plan to continue this work? It's been several months.

@m1kola
Copy link
Member

m1kola commented Oct 13, 2023

This PR has been silent for several months. I'm closing it, but please don't hesitate to re-open, if needed.

@m1kola m1kola closed this Oct 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants