-
Notifications
You must be signed in to change notification settings - Fork 55
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
feat(proxy): introduce datastore abstraction #425
base: main
Are you sure you want to change the base?
feat(proxy): introduce datastore abstraction #425
Conversation
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.
thank you @Al-Pragliola especially for as well 15e3ec2
can you also reflect any changes required in the https://github.com/kubeflow/model-registry/blob/main/docs/mr_go_library.md dev doc, please?
overall
/lgtm
At the moment there are no changes to clients using |
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.
thanks for the clarification and the refactor for this factory @Al-Pragliola
overall
/lgtm
and as mentioned I'd
/hold
this until we have at least dry-run the release process to implement pinning so we efficiently update the manifests accordingly to KF/manifests
MLMDPort: 9090, | ||
DatastoreHostname: "localhost", | ||
DatastorePort: 9090, | ||
DatastoreType: "mlmd", |
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.
should we have an enum for this?
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Signed-off-by: Alessio Pragliola <[email protected]>
Signed-off-by: Alessio Pragliola <[email protected]>
15e3ec2
to
f391159
Compare
New changes are detected. LGTM label has been removed. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Description
As proposed in issue #398 , I have introduced an abstraction to the underlying metadata storage service and changed the reference to the old --mlmd arguments, the next logical step should be to move the
pkg/core
tointernal
.UPDATE: Old arguments
--mlmd-hostname
--mlmd-port
have been reintroduced in order to prevent breaking changes in the manifests, but I have marked them as deprecated using cobra. This will show the following warning when an user run the proxy with them:How Has This Been Tested?
Tested with
make test-nocache
Tested the python client with
make test-e2e
Tested on a local kind cluster
expected result:
Merge criteria:
All the commits have been signed-off (To pass the
DCO
check)The commits have meaningful messages; the author will squash them after approval or in case of manual merges will ask to merge with squash.
Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
The developer has manually tested the changes and verified that the changes work.
Code changes follow the kubeflow contribution guidelines.