first code commit #1
Conversation
| value: "cincinnati-operator" | ||
| # pick a new value from https://quay.io/repository/app-sre/cincinnati?tag=latest&tab=tags | ||
| - name: OPERAND_IMAGE | ||
| value: "quay.io/app-sre/cincinnati:2873c6b" |
There was a problem hiding this comment.
Mental note: it would be nice if cincinnati had a versioned tag.
pkg/controller/cincinnati/new.go
Outdated
| } | ||
| } | ||
|
|
||
| // newPodForCR returns a busybox pod with the same name/namespace as the cr |
djzager
left a comment
djzager
left a comment
There was a problem hiding this comment.
Well done. This looks well put together and I like what you did with the options on the controller.
I tried to call out all of the names that I thought could be updated but I'm sure I missed some. I am in favor of just going ahead and giving it the full descriptive name so I don't have to look around and get context for what GB or PE could mean here.
Other than that the only structural thing I noticed had to do with updating the status. 👍
| @@ -1,15 +1,77 @@ | |||
| # Temporary Build Files | |||
| "strings" | ||
|
|
||
| // Import all Kubernetes client auth plugins (e.g. Azure, GCP, OIDC, etc.) | ||
| _ "k8s.io/client-go/plugin/pkg/client/auth" |
There was a problem hiding this comment.
for my own edification. What is going on here?
There was a problem hiding this comment.
I have no idea. This was part of the scaffolding from operator-sdk.
| // Add support for MultiNamespace set in WATCH_NAMESPACE (e.g ns1,ns2) | ||
| // Note that this is not intended to be used for excluding namespaces, this is better done via a Predicate | ||
| // Also note that you may face performance issues when using this with a high number of namespaces. | ||
| // More Info: https://godoc.org/github.com/kubernetes-sigs/controller-runtime/pkg/cache#MultiNamespacedCacheBuilder | ||
| if strings.Contains(namespace, ",") { | ||
| options.Namespace = "" | ||
| options.NewCache = cache.MultiNamespacedCacheBuilder(strings.Split(namespace, ",")) | ||
| } |
There was a problem hiding this comment.
Do we need this. I'm guessing it comes with operator-sdk scaffolding nowadays.
There was a problem hiding this comment.
You guessed right. I don't anticipate needing it, but it seems harmless so I'm inclined to leave it for now.
|
|
||
| var log = logf.Log.WithName("controller_cincinnati") | ||
|
|
||
| // Options holds settings for the reconciler |
| containers[i].Resources.Requests[k] = origVal | ||
| } | ||
| } | ||
| containers[i].VolumeMounts = original.VolumeMounts |
There was a problem hiding this comment.
Some reason these aren't up with the others like it?
There was a problem hiding this comment.
I think only because I went in the order that things were specified in the godocs. The Resources part above is part of the same flow of setting attributes, but required special care.
| @@ -0,0 +1,5 @@ | |||
| // +build tools | |||
There was a problem hiding this comment.
Do we want to leave this around in case we need it later?
There was a problem hiding this comment.
I'm not actually sure how we would use this. Do you know?
Co-Authored-By: David Zager <[email protected]>
|
@djzager I think I addressed all your feedback. Let me know if you have anything else. |
We've had a package docstring in both doc.go and register.go since v1alpha1 in 52a9765 (first code commit, 2020-04-03, openshift#1). But there's no reason to have a doc.go declaration when register.go already defines the same material. Also add a trailing period to the docstring sentence and remove the boilerplate comment, because this is a hand-maintained file, even if changes will be rare.
We've had a package docstring in both doc.go and register.go since v1alpha1 in 52a9765 (first code commit, 2020-04-03, openshift#1). But there's no reason to have a doc.go declaration when register.go already defines the same material. Also add a trailing period to the docstring sentence and capitalize the Cincinnati proper noun. And remove the boilerplate comment, because this is a hand-maintained file, even if changes will be rare.
5 participants
First pass. Obviously there is more to do, such as tests, productization, documentation, etc. but this should be a reasonable point to get feedback and then merge a starting point that works.