Sync registered model aliases via SetAlias/DeleteAlias in direct mode#4637
Open
varundeepsaini wants to merge 1 commit intodatabricks:mainfrom
Open
Sync registered model aliases via SetAlias/DeleteAlias in direct mode#4637varundeepsaini wants to merge 1 commit intodatabricks:mainfrom
varundeepsaini wants to merge 1 commit intodatabricks:mainfrom
Conversation
varundeepsaini
requested review from
andrewnester,
anton-107,
denik,
pietern and
shreyas-goenka
as code owners
varundeepsaini
force-pushed
the
fix-registered-model-aliases-direct
branch
from
ab92b8b to
034aac0
Compare
denik
reviewed
Mar 3, 2026
|
|
||
| // The Create API does not apply aliases, so we sync them separately. | ||
| if err := r.syncAliases(ctx, response.FullName, config.Aliases, nil); err != nil { | ||
| return "", nil, fmt.Errorf("failed to sync aliases: %w", err) |
Contributor
There was a problem hiding this comment.
Do we need to wrap the error? Our errors already printed with context such as API endpoint.
| remote, err := r.DoRead(ctx, id) | ||
| require.NoError(t, err) | ||
|
|
||
| assert.Len(t, remote.Aliases, 2) |
Contributor
There was a problem hiding this comment.
Can these be rewritten as acceptance test? You can capture requests there, run them against cloud, and also read remote state with databricks CLI.
|
|
||
| func (r *ResourceRegisteredModel) DoUpdate(ctx context.Context, id string, config *catalog.CreateRegisteredModelRequest, _ Changes) (*catalog.RegisteredModelInfo, error) { | ||
| // Fetch current remote state to determine which aliases to add/remove. | ||
| remote, err := r.client.RegisteredModels.Get(ctx, catalog.GetRegisteredModelRequest{ |
Contributor
There was a problem hiding this comment.
As a general guideline, DoUpdate should not read remote state. We already have it in the framework and we can pass it if it's needed. However, in this case we can probably get by reading passed Changes?
varundeepsaini
force-pushed
the
fix-registered-model-aliases-direct
branch
from
034aac0 to
cf22e16
Compare
varundeepsaini
force-pushed
the
fix-registered-model-aliases-direct
branch
from
cf22e16 to
86d4e85
Compare
Signed-off-by: Varun Deep Saini <varun.23bcs10048@ms.sst.scaler.com>
varundeepsaini
force-pushed
the
fix-registered-model-aliases-direct
branch
from
86d4e85 to
1300e29
Compare
|
An authorized user can trigger integration tests manually by following the instructions below: Trigger: Inputs:
Checks will be approved automatically on success. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #4012
Summary
aliasesfield on registered models, so aliases defined in bundle config were silently not applied in direct deployment mode.SetAlias/DeleteAliasAPI calls.IncludeAliaseson read so drift detection picks up alias changes.Test plan
TestAll/registered_modelsCRUD test passes