search

thoth-station / meteor-operator

Project Meteor Operator for OpenShift
GNU General Public License v3.0
4 stars 11 forks source link

fix/makefile install pipelines #143

Closed VannTen closed 1 year ago

VannTen commented 1 year ago

Fix #142

codificat commented 1 year ago

What about an alternative approach here:

That way, make deploy would behave the same way as an odh deployment via kfdef.

What I mean is to add this to the config/base/kustomization file:

bases:
  # ...
  # Install the meteor pipeline manifests
  - ../../pipelines/base

WDYT?

VannTen commented 1 year ago

That's way better, unless I misunderstand something about the structure in config/ @.*** wdyt ?) Thanks Pep !

codificat commented 1 year ago

I was a bit too optimistic with the proposal: it's not that simple, because when adding it to the overall kustomization, then the same name prefixes specified in kustomize apply to the pipelines.

The result is that the pipelines get installed like this:

$ oc get pipeline
NAME                           AGE
meteor-operator-cnbi-gitrepo   32m
meteor-operator-cnbi-import    32m

i.e. with the meteor-operator- prefix.

I don't think that's actually a bad thing per se, and we might actually prefer it!

However, the controller currently expects pipeline names without these prefixes, so this change would require an update to the controller when creating the PipelineRuns.

Should we add that change to the controller? It might be an opportunity / excuse to address https://github.com/thoth-station/meteor-operator/issues/97 at the same time (not completely directly related, but close enough).

VannTen commented 1 year ago

Oh yeah. I forgot that kustomize add names prefix everywhere. While I find this absurd (namespaces are precisely for this), that's trivial to fix. I wouldn't solve #97 at the same time though, that one requires us to answer https://github.com/thoth-station/meteor-operator/issues/97#issuecomment-1305494252 first.

VannTen commented 1 year ago

Btw, I'm currently having a discussion on that subject here: https://github.com/operator-framework/operator-sdk/issues/6186

VannTen commented 1 year ago

Actually it does not work either, because the Task also get prefixed and we're not using the prefix in the pipelines spec.

Honestly, I'd rather not use a prefix at all ; if there is good reason for it, it seems there is some undocumented stuff to skipPrefix on some resources.

@goern @codificat wdyt ?

codificat commented 1 year ago

/lgtm /approve

thanks!

sesheta commented 1 year ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: codificat

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: - ~~[OWNERS](https://github.com/thoth-station/meteor-operator/blob/main/OWNERS)~~ [codificat] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment