Show actionable errors for collaborative deployment scenarios #1386
Conversation
lennartkats-db
force-pushed
the
cp-better-errors
branch
from
dc33b28
to
c46ecde
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1386 +/- ##
==========================================
+ Coverage 52.25% 53.75% +1.50%
==========================================
Files 317 352 +35
Lines 18004 20410 +2406
==========================================
+ Hits 9408 10972 +1564
- Misses 7903 8636 +733
- Partials 693 802 +109 ☔ View full report in Codecov by Sentry. |
lennartkats-db
changed the title
lennartkats-db
changed the title
| return diag.Errorf("cannot write to deployment root (this can indicate a previous deploy was done with a different identity): %s", b.Config.Workspace.RootPath) | ||
| permissionError := filer.PermissionError{} | ||
| if errors.As(err, &permissionError) { | ||
| return permissions.ReportPermissionDenied(ctx, b, b.Config.Workspace.StatePath) |
There was a problem hiding this comment.
Filer now returns a proper PermissionError, simplifying things. And ReportPermissionDenied just adds a bit more context to the error about how to resolve it.
bundle/config/mutator/run_as.go
Outdated
| b.Config.GetLocation("resources.quality_monitors"), | ||
| b.Config.Workspace.CurrentUser.UserName, | ||
| identity, | ||
| ) |
There was a problem hiding this comment.
These diagnostics can be accumulated and returned together. In the (unlikely) scenario a user has defined a model serving endpoint and a quality monitor, they'll only see the error for their model serving endpoint and not the others.
There was a problem hiding this comment.
Sure. This is old code but I'll clean it up.
| Validate() error | ||
|
|
||
| json.Marshaler | ||
| json.Unmarshaler |
There was a problem hiding this comment.
This is just the campsite rule. Both of already exist in all struct implementing this interface; I'm just making them explicit.
There was a problem hiding this comment.
FYI, these are superfluous here. They implement this interface by embedding the Go SDK structs, but that is not what we need. See TestCustomMarshallerIsImplemented.
| if errors.As(err, ¬ExistsError) { | ||
| // If we still get a 404 error after the succesful mkdir, | ||
| // the problem is a permission error. | ||
| return PermissionError{absPath} |
There was a problem hiding this comment.
Yeah. This is the case where you the dir exists and you have 'view' access but not 'write' access. We previously couldn't recognize this case as a permission error.
There was a problem hiding this comment.
We need integration test coverage for this.
Let's figure out a way to do so, because we don't have any tests that use multiple identities at the moment, and we'll need that in order to test this branch (setup the tree to trigger this path).
Same holds for the permission checks in the rest of this PR. Without integration test coverage we can't guarantee continuity, or even that the assertions made continue to hold.
lennartkats-db
force-pushed
the
cp-better-errors
branch
2 times, most recently
from
8d26485
to
16b80ea
Compare
lennartkats-db
force-pushed
the
cp-better-errors
branch
from
16b80ea
to
7fff4bc
Compare
lennartkats-db
force-pushed
the
cp-better-errors
branch
from
737f02f
to
3c20dec
Compare
lennartkats-db
force-pushed
the
cp-better-errors
branch
from
3c20dec
to
b384b36
Compare
|
@pietern could you take another look? |
| Validate() error | ||
|
|
||
| json.Marshaler | ||
| json.Unmarshaler |
There was a problem hiding this comment.
FYI, these are superfluous here. They implement this interface by embedding the Go SDK structs, but that is not what we need. See TestCustomMarshallerIsImplemented.
|
|
||
| func (m *reportPermissionErrors) Name() string { | ||
| return "CheckPermissions" | ||
| } |
There was a problem hiding this comment.
Please consolidate the naming.
| // - assistance: advice on who to contact as to manage this project | ||
| func analyzeBundlePermissions(b *bundle.Bundle) (bool, string) { | ||
| canManageBundle := false | ||
| otherManagers := make(map[string]bool) |
There was a problem hiding this comment.
This is a set; you can use struct{} as value type or libs/set. The bool doesn't do anything.
| var managersSlice []string | ||
| for manager := range otherManagers { | ||
| managersSlice = append(managersSlice, manager) | ||
| } |
|
|
||
| assistance := "For assistance, contact the owners of this project." | ||
| if len(managersSlice) > 0 { | ||
| assistance = fmt.Sprintf("For assistance, users or groups with appropriate permissions may include: %s.", strings.Join(managersSlice, ", ")) |
| } | ||
|
|
||
| func ReportPermissionDenied(ctx context.Context, b *bundle.Bundle, path string) diag.Diagnostics { | ||
| log.Errorf(ctx, "Failed to update %v", path) |
There was a problem hiding this comment.
Ping. If this is not the case, please include a comment stating why this log statement shouldn't be removed.
| assert.EqualError(t, err, fmt.Sprintf("pipelines are not supported when the current deployment user is different from the bundle's run_as identity. Please deploy as the run_as identity. Please refer to the documentation at https://docs.databricks.com/dev-tools/bundles/run-as.html for more details. Location of the unsupported resource: %s:14:5. Current identity: [email protected]. Run as identity: my_service_principal", configPath)) | ||
| assert.ErrorContains(t, err, "pipelines do not support a setting a run_as user that is different from the owner.\n"+ | ||
| "Current identity: [email protected]. Run as identity: my_service_principal.\n"+ | ||
| "See https://docs", configPath) |
There was a problem hiding this comment.
configPath is no longer used.
| if errors.As(err, ¬ExistsError) { | ||
| // If we still get a 404 error after the succesful mkdir, | ||
| // the problem is a permission error. | ||
| return PermissionError{absPath} |
There was a problem hiding this comment.
We need integration test coverage for this.
Let's figure out a way to do so, because we don't have any tests that use multiple identities at the moment, and we'll need that in order to test this branch (setup the tree to trigger this path).
Same holds for the permission checks in the rest of this PR. Without integration test coverage we can't guarantee continuity, or even that the assertions made continue to hold.
|
@shreyas-goenka Can you take a pass as this as well? |
Changes
This adds diagnostics for collaborative (production) deployment scenarios, including:
/Users/Alice/.bundle.Tests
Unit tests, manual testing.