cli icon indicating copy to clipboard operation
cli copied to clipboard

Published 20 hours ago • verified publisher icon databricks

Show actionable errors for collaborative deployment scenarios

Open lennartkats-db opened this issue 1 year ago • 8 comments

Changes

This adds diagnostics for collaborative (production) deployment scenarios, including:

  • Bob deploys a bundle that is normally deployed by Alice, but this fails because Bob can't write to /Users/Alice/.bundle.
  • Charlie deploys a bundle that is normally deployed by Alice, but this fails because he can't create a new pipeline where Alice would be the owner.
  • Alice deploys a bundle where she didn't list herself as one of the CAN_MANAGE users in permissions. That can work, but is probably a mistake.

Tests

Unit tests, manual testing.

lennartkats-db avatar Apr 22 '24 07:04 lennartkats-db

Codecov Report

Attention: Patch coverage is 75.56818% with 43 lines in your changes are missing coverage. Please review.

Project coverage is 53.75%. Comparing base (e22dd8a) to head (0b9feab). Report is 125 commits behind head on main.

Files Patch % Lines
bundle/permissions/permission_diagnostics.go 87.60% 11 Missing and 4 partials :warning:
libs/filer/workspace_files_client.go 0.00% 14 Missing :warning:
bundle/config/mutator/run_as.go 81.81% 6 Missing :warning:
bundle/deploy/terraform/apply.go 0.00% 4 Missing :warning:
libs/filer/filer.go 0.00% 4 Missing :warning:
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

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov-commenter avatar Jun 02 '24 12:06 codecov-commenter

@pietern could you take another look?

lennartkats-db avatar Jul 11 '24 13:07 lennartkats-db

@shreyas-goenka Can you take a pass as this as well?

pietern avatar Jul 15 '24 07:07 pietern

Missed the ping, taking a look.

shreyas-goenka avatar Jul 22 '24 09:07 shreyas-goenka

@shreyas-goenka

Does this work end to end? Can Bob collaborate on a DAB deployed by Alice if he has CAN_MANAGE?

I recall there being some validation on the Terraform provider side that would fail even if Bob has CAN_MANAGE permissions set.

Yes, though that scenario requires the use of an explicit "OWNER: Alice" permission. Which is supported via https://github.com/databricks/cli/pull/1387.

lennartkats-db avatar Jul 31 '24 08:07 lennartkats-db

We discussed this a while back; I'd like to get integration test coverage for the permission errors on the filer. It's implemented only for the workspace files client now, not for the others (for ex the files API).

pietern avatar Sep 03 '24 09:09 pietern

@pietern

We discussed this a while back; I'd like to get integration test coverage for the permission errors on the filer. It's implemented only for the workspace files client now, not for the others (for ex the files API).

Didn't we conclude that we would just not change the logic of the filer? I reverted that change.

lennartkats-db avatar Sep 09 '24 07:09 lennartkats-db

@pietern Updated the PR, processing all the new comments. I'd really like to get this merged.

lennartkats-db avatar Sep 09 '24 07:09 lennartkats-db

I'll submit a separate PR to remove the remaining IS_OWNER.

pietern avatar Oct 10 '24 11:10 pietern