-
Notifications
You must be signed in to change notification settings - Fork 111
Add support for registered models for direct deployment #3670
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
Conversation
|
| Deleting files... | ||
| Destroy complete! | ||
|
|
||
| >>> [CLI] schemas delete mycatalog-[UNIQUE_NAME].myschema-[UNIQUE_NAME] --force |
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.
As discussed offline, would be great to use acceptance/bundle/resources/<resource_name>/ schema.
bundle/deploy is really focussed on deploy command but we're also interested in plan/json plan/destroy/summary.
another aspect is to use smaller tests which is good for perf and debugging:
See how this test is organized:
~/work/cli/acceptance/bundle/resources % tree -d volumes
volumes
├── change-comment
├── change-name
├── change-schema-name
├── remote-change-name
├── remote-delete
└── set-storage-location
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.
I'll address this in a followup PR.
| @@ -0,0 +1,2 @@ | |||
| Cloud = true | |||
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.
Appreciate making it both local and integration test, that adds to confidence.
| trap cleanup EXIT | ||
|
|
||
| deploy_registered_model() { | ||
| trace $CLI bundle plan |
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.
recording "bundle debug plan" would be nice as well.
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.
I'll address this in a followup PR.
|
|
||
| deploy_registered_model() { | ||
| trace $CLI bundle plan | ||
| trace $CLI bundle deploy |
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.
recording non-GET requests would be useful to ensure terraform and direct implementations match. If there is too much difference in payload, it can either be cleaned up or we can just record paths without the payload.
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.
I'll address this in a followup PR.
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.
The implementation looks solid, there are comments on test organization, can be done here or as a follow up.
Changes
This PR adds support for registered models, matching the same behaviour as TF. This PR also adds test server handles for registered models and UC catalogs, as was necessary for the integration test.
Why
To complete coverage for direct deployment.
Tests
New integration test.