-
Notifications
You must be signed in to change notification settings - Fork 12
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
Implemented controller for NATS CR #19
Conversation
@@ -65,7 +65,7 @@ vet: ## Run go vet against code. | |||
go vet ./... | |||
|
|||
.PHONY: test | |||
test: manifests go-gen generate fmt vet envtest lint-thoroughly ## Run tests. | |||
test: manifests generate fmt vet envtest lint-thoroughly ## Run tests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
go-gen
is already called inside generate
Dockerfile
Outdated
@@ -30,6 +30,8 @@ RUN CGO_ENABLED=0 GOOS=${TARGETOS:-linux} GOARCH=${TARGETARCH} go build -a -o ma | |||
FROM gcr.io/distroless/static:nonroot | |||
LABEL source = git@github.com:kyma-project/nats-manager.git | |||
|
|||
ARG NATS_CHARTS_DIR="/resources/nats" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is this used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
@@ -22,7 +22,7 @@ limitations under the License. | |||
package v1alpha1 | |||
|
|||
import ( | |||
"k8s.io/apimachinery/pkg/apis/meta/v1" | |||
v1 "k8s.io/apimachinery/pkg/apis/meta/v1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A bit strange code generation 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no idea, its gone now somehow.
config/manager/manager.yaml
Outdated
@@ -69,6 +69,9 @@ spec: | |||
image: controller:latest | |||
imagePullPolicy: Always | |||
name: manager | |||
env: | |||
- name: NATS_CHARTS_DIR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NATS_CHARTS_DIR
-> NATS_CHART_DIR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
setupLog.Error(err, "unable to get configs from env") | ||
os.Exit(1) | ||
} | ||
|
||
// @TODO: Re-check logger setup and init |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this todo still relevant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, I added a todo here #8
- apiGroups: | ||
- "" | ||
resources: | ||
- configmaps | ||
verbs: | ||
- create | ||
- delete | ||
- get | ||
- list | ||
- patch | ||
- update | ||
- watch | ||
- apiGroups: | ||
- "" | ||
resources: | ||
- secrets | ||
verbs: | ||
- create | ||
- delete | ||
- get | ||
- list | ||
- patch | ||
- update | ||
- watch | ||
- apiGroups: | ||
- "" | ||
resources: | ||
- services | ||
verbs: | ||
- create | ||
- delete | ||
- get | ||
- list | ||
- patch | ||
- update | ||
- watch | ||
- apiGroups: | ||
- apps/v1 | ||
resources: | ||
- statefulsets | ||
verbs: | ||
- create | ||
- delete | ||
- get | ||
- list | ||
- patch | ||
- update | ||
- watch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Too much power, do we really need them all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will be re-visited in this ticket: #21
// Check if istio is enabled in cluster | ||
istioExists, err := r.kubeClient.DestinationRuleCRDExists(ctx) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As per the comment, this checks if istio is enabled in the cluster or not. I'm not sure if that's the right way to do it. I think we need to check if istio as a Kyma module is ready or not, by checking istio CR status.state. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, the only thing we care about is the DestinationRule
exists or not, so we can create CR for it. Checking the kyma module will make it more coupled to Istio module. Also, what if user has it own deployment of istio resources.
|
||
// @TODO: Provide the overrides in component.Configuration | ||
instance.Configuration = map[string]interface{}{ | ||
"istio.enabled": istioExists, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What will this do in case of true/false?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If true
, then it will create DestinationRule
CR.
// @TODO: Provide the overrides in component.Configuration | ||
instance.Configuration = map[string]interface{}{ | ||
"istio.enabled": istioExists, | ||
"auth.rotatePassword": accountSecret == nil, // do not recreate secret if it exists |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we sure this will create a secret that matches the accountSecretName
and nats.Namespace
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name format is the same in NATS helm chart. If someone changes the name format in helm, the pre-main tests should fail.
func (r *Reconciler) loggerWithNATS(nats *natsv1alpha1.NATS) *zap.SugaredLogger { | ||
return r.logger.With( | ||
"kind", nats.GetObjectKind().GroupVersionKind().Kind, | ||
"version", nats.GetGeneration(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"version"
-> "generation"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
/cla-recheck |
Description
Changes proposed in this pull request:
Related issue(s)