chore: separate templates #932
Conversation
There was a problem hiding this comment.
I have some suggestions for the structure of the code, but I am not also quite sure about the directories. If we are moving templates around maybe we can also improve the folders.
In particular for KServe I think it would be better to group templates by capability/infra, so that would look like:
├── servicemesh
│ ├── activator-envoyfilter.tmpl.yaml
│ ├── envoy-oauth-temp-fix.tmpl.yaml
│ ├── grpc-envoyfilter-temp-fix.tmpl.yaml
│ ├── kserve-predictor-authorizationpolicy.tmpl.yaml
│ ├── routing
│ │ ├── istio-ingress-gateway.tmpl.yaml
│ │ ├── istio-local-gateway.yaml
│ │ └── local-gateway-svc.tmpl.yaml
│ └── z-migrations
│ └── kserve-predictor-authorizationpolicy.patch.tmpl.yaml
├── serving-install
│ ├── knative-serving.tmpl.yaml
│ └── service-mesh-subscription.tmpl.yaml
├── serving-istio-gateways
└── serving-net-istio-secret-filtering.patch.tmpl.yaml
With regards to fs.FS and kyaml's filesys.FileSystem or in general support for Kustomize. In a retrospect, I think making it available right now might be a bit premature. We do not have the whole thing in parity with deploy.go (especially in terms of feature create/update/remove path from the reconcile perspective). At the time we start looking at how to unify the approach we can bring this code back. Perhaps best for now is to make a separate PR first removing this part (so we can restore easier at later stage) and then get this one in.
| Manifests( | ||
| path.Join(feature.KServeDir, "grpc-envoyfilter-temp-fix.tmpl"), | ||
| path.Join(serviceMeshDir, "grpc-envoyfilter-temp-fix.tmpl.yaml"), |
There was a problem hiding this comment.
This works, but it seems we missed one thing in the function below:
func isTemplateManifest(path string) bool {
return strings.Contains(path, ".tmpl")
}
this would allow /opt/my.tmpl/raw.yaml to be treated as template. Better to get base path I think. Maybe also check .tmpl. (and the same for .patch.)
There was a problem hiding this comment.
Agree for checking basepath there - a945320
But I think checking for .patch. or .tmp. would be redundant since .patch. contains .patch
There was a problem hiding this comment.
My thinking here is that .tmpl. is more explicit in terms of format
There was a problem hiding this comment.
Ah, I understand now, we can force our naming convention with this. Will do.
Co-authored-by: Bartosz Majsak <bartosz.majsak@gmail.com>
| Manifests( | ||
| path.Join(feature.ServerlessDir, "serving-net-istio-secret-filtering.patch.tmpl"), | ||
| path.Join(baseDir, "serving-net-istio-secret-filtering.patch.tmpl.yaml"), |
There was a problem hiding this comment.
Worth making it part of Templates struct? I also wonder if this is a good name. After all it's not only templates we are keeping there.
There was a problem hiding this comment.
renamed the struct and included Resources.BaseDir on top of the const baseDir to be used (so the const can still be used in creating the fields for the subpaths
|
@bartoszmajsak re-requested review as I've re-done the kserve smoke test as discussed and it worked |
There was a problem hiding this comment.
LGTM
I intentionally put it on hold as I would like @israel-hdez to sign off on this one before we merge it.
|
/retest |
There was a problem hiding this comment.
I intentionally put it on hold as I would like @israel-hdez to sign off on this one before we merge it.
You have my 👍. All works fine for me after the changes.
I'm dropping two non-blocking comments.
There was a problem hiding this comment.
AFAIK, at the moment, these resources related to metrics-collection are more because of a requirement from Model Serving.
I don't know if there is a platform-wide requirement for this. So, with this new file organization, I don't know, nor I'm sure if this is the right place for this.
There was a problem hiding this comment.
Good point, we will take care of it when moving stuff around, though the feature is already in DSCI part at the moment :)
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bartoszmajsak, israel-hdez, zdtsw The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-merge-bot
bot
merged commit 9df56a0
into
opendatahub-io:incubation
* init commit * move const, move comment * remove unused fixture * Update components/kserve/feature_templates.go Co-authored-by: Bartosz Majsak <bartosz.majsak@gmail.com> * make template structure consistent from review * only check basepath for tmpl * re-add template test * make ManifestSource required to call Manifest * fix linter? * make review changes * lint post rebase * preserve prior manifest order --------- Co-authored-by: Bartosz Majsak <bartosz.majsak@gmail.com>
Description
RHOAIENG-4604
This PR separates out the templates that are embedded and applied as part of Feature manifests. The goal here is to make it easier to find the relevant templates for the component they are used, so we move templates from a central directory under
pkg/feature/templatesinto their own/templatesdirectories, IEcomponents/kserve/templatesnow stores the templates used in deploying kserve with service mesh andcontrollers/dscinitialization/templatesholds the templates used in deploying service mesh.This PR also renames our
.tmplfiles to.tmpl.yamlso that IDEs will recognize and format these files accordingly.Note that this changes the behavior, so that when supplying a Feature with Manifests(paths) to files within an embedded file system, one has to first set the ManifestSource() to the embeddedFS.
How Has This Been Tested?
Tested on CRC with image
quay.io/cgarriso/opendatahub-operator:dev-separate-templatesand ensured that kserve + dsci features still applied successfully.Merge criteria: