-
Notifications
You must be signed in to change notification settings - Fork 835
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Extend CRD: allow to define storageInitializerImage in the graph definition #2937
Extend CRD: allow to define storageInitializerImage in the graph definition #2937
Conversation
/test integration |
storageInitializerImage
to the PredictiveUnit
definition
storageInitializerImage
to the PredictiveUnit
definition
Tue Feb 9 10:39:22 UTC 2021 impatient try |
Tue Feb 9 10:39:23 UTC 2021 impatient try |
/test integration |
This should be ready for review now. |
Tue Feb 9 18:15:15 UTC 2021 impatient try |
Tue Feb 9 18:15:16 UTC 2021 impatient try |
/test integration |
Thu Feb 11 11:41:24 UTC 2021 impatient try |
Thu Feb 11 11:41:28 UTC 2021 impatient try |
Need to rebase and rerun tests |
IMAGE_VERSION := $(shell cat ../../version.txt) | ||
IMAGE_NAME=seldonio/rclone-init-container-example | ||
# IMAGE_VERSION := $(shell cat ../../version.txt) | ||
IMAGE_VERSION=0.1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is image version not based on release. Is it assumed it is unlikely to change be effected by release. If so maybe remove the commented line?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is only an example that actually has no dependency on the in-repo code. It just changes the entrypoint
of the upstream image:
FROM rclone/rclone:latest
ENTRYPOINT ["rclone", "copy"]
When later we will add a proper rclone-based alternative to storage.py (not just an example of custom init container) then I guess it will make sense to version it with ../../version.txt
.
operator/Makefile
Outdated
|
||
manifests_all: manifests manifests_v1 | ||
|
||
install_all: install install_v1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
install all doesn't make sense to me - you either need to install pre 1.18 CRD or post 1.18 CRD
@@ -129,9 +133,10 @@ func (pi *PrePackedInitialiser) addTFServerContainer(mlDepSpec *machinelearningv | |||
} | |||
|
|||
envSecretRefName := extractEnvSecretRefName(pu) | |||
storageInitializerImage := extractStorageInitializerImage(pu) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we need a separate function for this as it just returns the value from pu
failed to trigger Pull Request pipeline
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a few small changes
/test integration |
/test notebooks |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cliveseldon 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 |
What this PR does / why we need it:
This allows to define image used by the
Storage Initializer
as part of thePredictiveUnit
definition -graph
section in theCR
e.g.This allow to have elegant way to overwrite used
storageInitializerImage
on per-deployment basis.Which issue(s) this PR fixes:
Fixes #2821
Special notes for your reviewer:
To be finished:
Does this PR introduce a user-facing change?: