acc: auto clean up old wheels #2759
Conversation
denik
requested review from
andrewnester,
anton-107,
pietern and
shreyas-goenka
as code owners
acceptance/acceptance_test.go
Outdated
| func buildDatabricksBundlesWheel(t *testing.T, buildDir string) (string, error) { | ||
| func buildDatabricksBundlesWheel(t *testing.T, buildDir string) string { | ||
| // Clean up directory, remove all but the latest wheel | ||
| _ = findWheel(t, buildDir) |
There was a problem hiding this comment.
Why do this twice? The build will create a new one anyway, so the second call will clear out the old stuff.
There was a problem hiding this comment.
I don't want to make an assumption that build command always updates mtime.
I also don't want to delete the one wheel that is most likely to be reused. So I'm keeping last mtime wheel there.
There was a problem hiding this comment.
Added a comment with an explanation. I believe this approach makes least assumptions and so is most resilient.
There was a problem hiding this comment.
BTW, I checked 'uv build' does not update mtime if up to date whl is already there.
| return "", fmt.Errorf("failed to read directory %s: %s", buildDir, err) | ||
| latestWheel := findWheel(t, buildDir) | ||
| if latestWheel == "" { | ||
| t.Errorf("databricks-bundles wheel not found in %s", buildDir) |
There was a problem hiding this comment.
This does not call t.FailNow(), so it continues execution. Is this intentional?
There was a problem hiding this comment.
Yes, added a comment there. Only a couple of tests need that wheel, no need to fail all of them.
There was a problem hiding this comment.
Do we need such fallback? It will be harder to debug if anything goes wrong. CLI commands will also fail in a very weird way
There was a problem hiding this comment.
This will make DATABRICKS_BUNDLES_WHEEL undefined and affected bash scripts will fail with clear message because we run them with -e.
kanterov
left a comment
kanterov
left a comment
There was a problem hiding this comment.
It can be a surprise that findWheel removes wheels in addition to finding them.
I think it might be easier to follow if we create a function that returns a list of wheels. We can delete all of them before we run build command. We don't need to keep binary as we do for CLI.
I renamed it and added a comment re what it does. |
general rule we cache all build artifacts in acceptance test runner.
Changes
Automatically clean up old wheels instead of aborting the tests.
Tests
Manually: