Further fluxv2 plugin features: CreateInstalledPackage initial implementation #3337
Conversation
| @@ -26,6 +27,11 @@ rules: | |||
| - apiGroups: [""] | |||
| resources: ["secrets"] | |||
| verbs: ["get", "list"] | |||
| # needed by fluxv2 plug-in to check whether flux CRDs have been installed | |||
| # TODO (gfichtenholt) possibly surround this with "if fluxv2 plugin is enabled" | |||
There was a problem hiding this comment.
Yes, but not as a condition on this ClusterRole... this cluster role is currently intended to be used in development only (ie. the binding below is only done when unsafeUseDemoSA is set to true), hence the name being kubeapps-apis-dev above. The helm (direct) and kapp-controller plugins are able to use the users' credential for all interactions when requests come from the client.
But in your case, because you're caching data and interacting with the cluster outside of user requests, you need to ensure that extra RBAC required by your plugin is available to the service account. So longer term (not for this PR), I'd separate it out into a different cluster-role (perhaps in a different file) and put the condition there.
| @@ -160,6 +162,33 @@ func newCacheWithRedisClient(config cacheConfig, redisCli *redis.Client, waitGro | |||
| return &c, nil | |||
| } | |||
|
|
|||
| func (c NamespacedResourceWatcherCache) isGvrValid() error { | |||
| if c.config.gvr.Empty() { | |||
| return status.Errorf(codes.FailedPrecondition, "server configured with empty GVR") | |||
There was a problem hiding this comment.
Not sure why grpc status errors are used in this function... the call-site isn't returning a response I don't think? If it's just on startup, normal errors are fine (though I don't see any issue using grpc ones).
There was a problem hiding this comment.
it had something to do with unit tests, I think. Not important enough now to go into. Will change if/when needed
| // HACK: just for now assume HelmRelease CRD will live in the kubeapps namespace | ||
| kubeappsNamespace := os.Getenv("POD_NAMESPACE") | ||
| if kubeappsNamespace == "" { | ||
| // unit tests don't have this env var set |
There was a problem hiding this comment.
You can set an env var for the duration of a test - see https://stackoverflow.com/a/66061355 (nice, I didn't know about the new testing.T.SetEnv)
There was a problem hiding this comment.
oh, cool. Will use that, thanks
| Context: &corev1.Context{Namespace: name.Namespace}, | ||
| Identifier: name.Name, | ||
| }, nil | ||
| } |
There was a problem hiding this comment.
Nice - that function is simpler than I expected it to be, due to the work you've done previously for fetchChartFromCache etc. :)
There was a problem hiding this comment.
thanks for the review.
A few things in this PR: