Skip to content
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

Exporter: detect & handle deleted workspace objects (notebooks/files/directories) when running in incremental mode #3225

Merged
merged 8 commits into from
Feb 12, 2024

Conversation

alexott
Copy link
Contributor

@alexott alexott commented Feb 7, 2024

Changes

The initial version of code for detection & handling of removed workspace objects (notebooks, workspace files, and directories) and associated permissions... Renames are handled as (delete/create new)...

Tests

  • make test run locally
  • relevant change in docs/ folder
  • covered with integration tests in internal/acceptance
  • relevant acceptance tests are passing
  • using Go SDK

@alexott alexott added exporter TF configuration generator do-not-merge labels Feb 7, 2024
@alexott alexott requested review from a team as code owners February 7, 2024 09:40
@alexott alexott requested review from mgyucht and removed request for a team February 7, 2024 09:40
@codecov-commenter
Copy link

codecov-commenter commented Feb 7, 2024

Codecov Report

Attention: 19 lines in your changes are missing coverage. Please review.

Comparison is base (662a0bd) 83.60% compared to head (b3a035b) 83.77%.
Report is 14 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3225      +/-   ##
==========================================
+ Coverage   83.60%   83.77%   +0.17%     
==========================================
  Files         169      170       +1     
  Lines       15094    15301     +207     
==========================================
+ Hits        12619    12819     +200     
+ Misses       1735     1731       -4     
- Partials      740      751      +11     
Files Coverage Δ
exporter/importables.go 78.76% <ø> (+0.06%) ⬆️
exporter/util.go 79.73% <0.00%> (-0.36%) ⬇️
exporter/context.go 82.58% <90.97%> (+0.70%) ⬆️

... and 5 files with indirect coverage changes

@alexott alexott changed the title WIP: Exporter: detect & handle deleted workspace objects (notebooks/files/directories) when running in incremental mode Exporter: detect & handle deleted workspace objects (notebooks/files/directories) when running in incremental mode Feb 7, 2024
Copy link
Contributor

@tanmay-db tanmay-db left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @alexott, I have some comments, mentioned them. Right now, we don't have E2E tests for exporter but just to check, has this been tested E2E?

docs/guides/experimental-exporter.md Outdated Show resolved Hide resolved
exporter/context.go Outdated Show resolved Hide resolved
exporter/context.go Outdated Show resolved Hide resolved
@@ -499,6 +529,110 @@ func (ic *importContext) Run() error {
return nil
}

func isSupportedWsObject(obj workspace.ObjectStatus) bool {
return obj.ObjectType == workspace.Directory || obj.ObjectType == workspace.Notebook || obj.ObjectType == workspace.File
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having a list of supported objects and then checking in that would be better, we could extend it to other objects in future.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reimplemented as switch, but really we can detect changes only in specific resources...

case workspace.Notebook:
rtype = "databricks_notebook"
default:
log.Printf("[WARN] Unsupported WS object type: %s in obj %v", obj.ObjectType, obj)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we fail here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's too late to fail - it's better to don't some missing code that could be added manually, than fail multi-hour work. Although, this block is just defensive piece - because we have filters earlier we don't reach it.

exporter/context.go Outdated Show resolved Hide resolved
p := ic.oldWorkspaceObjectMapping[object.ObjectID]
if p == "" || p == object.Path {
log.Printf("[DEBUG] skipping '%s' that was modified at %d (last active=%d)",
object.Path, modifiedAt, updatedSinceMs)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is last active same as last updated?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. Last Active - when the exporter ran last time...

@alexott alexott requested a review from tanmay-db February 12, 2024 18:53
@alexott
Copy link
Contributor Author

alexott commented Feb 12, 2024

Most of the tests in exporter_test are running this tool as a command-line app. I'm testing all things manually with manual checks for generated code. Setting up fully automated testing is a significant task on its own (especially on huge workspaces), and will consume a lot of resources that I don't have.

@tanmay-db tanmay-db added this pull request to the merge queue Feb 12, 2024
Merged via the queue into main with commit a644c6a Feb 12, 2024
5 checks passed
@tanmay-db tanmay-db deleted the exporter-handle-deleted-ws-objects branch February 12, 2024 19:01
@tanmay-db tanmay-db mentioned this pull request Feb 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
exporter TF configuration generator
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants