fix: make deploy target #401
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sthaha
reviewed
Jun 10, 2024
| @@ -2,6 +2,7 @@ apiVersion: apiextensions.k8s.io/v1 | |||
| kind: CustomResourceDefinition | |||
| metadata: | |||
| annotations: | |||
| cert-manager.io/inject-ca-from: kepler-operator-system/kepler-operator-serving-cert | |||
There was a problem hiding this comment.
How does this behave on OpenShift that has cert-manager installed?
sthaha
reviewed
Jun 10, 2024
Makefile
Outdated
Comment on lines
251
to
253
| deploy: install cmctl ## Deploy controller to the K8s cluster specified in ~/.kube/config. | ||
| kubectl apply --server-side --force-conflicts -f \ | ||
| https://github.com/jetstack/cert-manager/releases/download/v$(CERTMANAGER_VERSION)/cert-manager.yaml |
There was a problem hiding this comment.
Imagine running this on k8s with a cert-manager already installed and configured? Can this potentially break their system?
IMO, we should first validate if certmanager exists and install it only if there isn't any.
Additionally, consider exiting with error if cert-manager is absent.
We can also provide install-certmanager helper to make life easier for vanilla k8s installation.
sthaha
reviewed
Jun 10, 2024
hack/cluster.sh
Outdated
| @@ -31,7 +31,6 @@ declare -r PROJECT_ROOT | |||
| declare -r TMP_DIR="$PROJECT_ROOT/tmp" | |||
| declare -r DEV_CLUSTER_DIR="$TMP_DIR/local-dev-cluster" | |||
| declare -r BIN_DIR="$TMP_DIR/bin" | |||
| declare -r OPERATOR_SDK_VERSION=${OPERATOR_SDK_VERSION:-v1.27.0} | |||
There was a problem hiding this comment.
any reason to remove this?
There was a problem hiding this comment.
wasn't used in the script anywhere
vprashar2929
force-pushed
the
fix-make-dep
branch
from
ef0541a to
a9dab7f
Compare
This PR address the issue sustainable-computing-io#381 by: * Enabling cert-manager that is necessary by webhooks during the deployment of Operator on k8s. * Added a separate config for k8s that contains the manifests that are specific for deploying on k8s. * `config/default` points to OpenShift specific manifests * `config/k8s` points to manifests specific for k8s * Added overlays in manager to isolate k8s and OpenShift Signed-off-by: Vibhu Prashar <[email protected]>
vprashar2929
force-pushed
the
fix-make-dep
branch
from
a9dab7f to
2ce068f
Compare
vprashar2929
commented
Jun 11, 2024
There was a problem hiding this comment.
Deleted this file as it wasn't used anywhere
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR address the issue #381 by:
Enabling cert-manager that is necessary by webhooks during the
deployment of Operator on k8s.
Added a separate config for k8s that contains the manifests that
are specific for deploying on k8s.
config/defaultpoints to OpenShift specific manifestsconfig/k8spoints to manifests specific for k8sAdded overlays in manager to isolate k8s and
OpenShift