BundleDeployment: add and implement required installNamespace field #864
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #864 +/- ##
==========================================
- Coverage 37.28% 37.27% -0.01%
==========================================
Files 9 9
Lines 853 845 -8
==========================================
- Hits 318 315 -3
+ Misses 491 486 -5
Partials 44 44 ☔ View full report in Codecov by Sentry. |
joelanford
force-pushed
the
install-namespace
branch
4 times, most recently
from
6a1a5fe to
168a689
Compare
openshift-merge-robot
added
the
needs-rebase
joelanford
force-pushed
the
install-namespace
branch
2 times, most recently
from
089c30b to
1828b3b
Compare
openshift-merge-robot
removed
the
needs-rebase
joelanford
force-pushed
the
install-namespace
branch
from
1828b3b to
dc03d98
Compare
| if strings.Contains(err.Error(), "no matches for kind") { | ||
| return true | ||
| } | ||
| return strings.Contains(err.Error(), "the server could not find the requested resource") |
There was a problem hiding this comment.
Was it intentional to add this? Could not find requested resource can have many reasons - given this is called after helm install, could there be any side effects?
There was a problem hiding this comment.
I noticed some tests failing (maybe flaking?) when I was running the e2e's. We've seen similar things before (and I can't remember specifics now), but I think the difference between "no matches for kind" and "the server could not find the requested resource" is maybe coming from the dynamic client's request to the apiserver vs the discovery interface's lookup of the rest mapping.
I don't think there are any side-effects in user facing behavior though. We just have this custom error type so that we can assert on it.
| @@ -19,7 +20,7 @@ const ( | |||
| ) | |||
|
|
|||
| func HandleBundleDeployment(ctx context.Context, fsys fs.FS, bd *rukpakv1alpha2.BundleDeployment) (*chart.Chart, chartutil.Values, error) { | |||
| plainFS, err := convert.RegistryV1ToPlain(fsys, bd.Spec.WatchNamespaces) | |||
| plainFS, err := convert.RegistryV1ToPlain(fsys, bd.Spec.InstallNamespace, []string{metav1.NamespaceAll}) | |||
There was a problem hiding this comment.
I assume removal of watchNamespace field would be a follow up?
There was a problem hiding this comment.
You mean the RegistryV1ToPlain function accepting a watchNamespaces parameter? I left that in intentionally. I assume we'll still need all of that underlying logic in order to support all install modes via the Extension API.
77534a3
In order to make it possible to add an
InstallNamespacefield in the operator-controller project'sClusterExtensionspec, we need to add it here first (because the embedding of rukpak logic in operator-controller is still ongoing).The logic here configures the Helm client to use the InstallNamespace as the context for the templating engine and k8s client, the result being similar to using
helm --namespace <installNamespace>. The exception is that rukpak continues to store the helm release secret in its system namespace.This commit pins
rukpakto an untagged commit onmainin thehelm-operator-pluginsrepo. I can tag that commit if reviewers want to stick with a tagged release for helm-operator-plugins.