RHOAIENG-1724-Add V2_LAUNCHER_IMAGE and V2_DRIVER_IMAGE for dspa api server #543
Conversation
|
A new image has been built to help with testing out this PR: To use this image run the following: cd $(mktemp -d)
git clone git@github.com:opendatahub-io/data-science-pipelines-operator.git
cd data-science-pipelines-operator/
git fetch origin pull/543/head
git checkout -b pullrequest 197acc7e4965f2259bffa23cd661d442a04fb517
make deploy IMG="quay.io/opendatahub/data-science-pipelines-operator:pr-543"More instructions here on how to deploy and test a Data Science Pipelines Application. |
|
Change to PR detected. A new PR build was completed. |
VaniHaripriya
force-pushed
the
RHOAIENG-1724
branch
from
0ae519b
to
7690a06
Compare
|
Change to PR detected. A new PR build was completed. |
…server Updated cases to resolve functest errors Removed unwanted env vars
VaniHaripriya
force-pushed
the
RHOAIENG-1724
branch
from
8c8dd79
to
35ae5d7
Compare
|
Change to PR detected. A new PR build was completed. |
controllers/dspipeline_params.go
Outdated
| APIServerKFPLauncherImagePath := p.GetImageForComponent(dsp, config.APIServerKFPLauncherImagePath, config.APIServerKFPLauncherImagePathV2Argo, config.APIServerKFPLauncherImagePathV2Tekton) | ||
| APIServerDriverImagePath := p.GetImageForComponent(dsp, config.APIServerDriverImagePath, config.APIServerDriverImagePathV2Argo, config.APIServerDriverImagePathV2Tekton) |
There was a problem hiding this comment.
we don't need to set the v2 tekton, and v1 parameters, we can just directly use
config.APIServerDriverImagePathV2Argo and config.APIServerKFPLauncherImagePathV2Argo, if you notice in the go template, these images are only utilized for v2 argo
api/v1alpha1/dspipeline_types.go
Outdated
| KFPLauncherImage string `json:"kfpLauncherImage,omitempty"` | ||
| DriverImage string `json:"driverImage,omitempty"` |
There was a problem hiding this comment.
these are argo specific, I'm wondering if we should put this in a sub argo field like:
spec:
apiServer:
...
V2Argo:
KFPLauncherImage: ...
DriverImage: ...
@gregsheremeta / @gmfrasca thoughts?
There was a problem hiding this comment.
I'm not sure if nesting this would add some unnessesary complexity but I do like the idea of denoting they're Argo related. Perhaps naming them ArgoLauncherImage and ArgoDriverImage would be a good compromise?
There was a problem hiding this comment.
another consideration for @HumairAK 's suggestion would be that we would need to/should move the Tekton-specific images (ie MoveResultsImage) to their own sub-item
There was a problem hiding this comment.
I like both of Giulio's suggestions. Nothing else to add.
There was a problem hiding this comment.
let's just go with Argo* naming and not do the nesting
|
Change to PR detected. A new PR build was completed. |
|
/lgtm |
|
Change to PR detected. A new PR build was completed. |
1 similar comment
|
Change to PR detected. A new PR build was completed. |
Fix- Funnc test errors
|
Change to PR detected. A new PR build was completed. |
VaniHaripriya
force-pushed
the
RHOAIENG-1724
branch
from
645f498
to
5181c4b
Compare
|
Change to PR detected. A new PR build was completed. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: amadhusu, gmfrasca 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 |
The issue resolved by this Pull Request:
Resolves RHOAIENG-1724
Description of your changes:
Added V2_LAUNCHER_IMAGE and V2_DRIVER_IMAGE params for dspa api server so that users can specify the kfp launcher and driver image when submitting a DSPA.
Testing instructions
Deploy DSPO
Create DSPA instance using dspa_simple_v2.yaml file which has the newly added fields kfpLauncherImage and driverImage.
Checklist