Add acceptance tests for bundle deploy #2254
Conversation
kanterov
requested review from
pietern,
andrewnester,
shreyas-goenka and
denik
as code owners
acceptance/acceptance_test.go
Outdated
| t.Fatalf("Failed to download terraform: %v", err) | ||
| } | ||
|
|
||
| repls.SetPath(tfExecPath, "$DATABRICKS_TF_EXEC_PATH") |
There was a problem hiding this comment.
Looks like it didn't work
| http.Error(w, err.Error(), http.StatusInternalServerError) | ||
| return | ||
| if errors.Is(err, NotFoundError) { | ||
| payload := `{"error_code":"RESOURCE_DOES_NOT_EXIST","message":"Does not exist"}` |
There was a problem hiding this comment.
Not sure this is the correct way. But we need to send the correct payload back, or SDK can't parse it.
| @@ -65,6 +66,34 @@ func TestInprocessMode(t *testing.T) { | |||
| require.Equal(t, 1, testAccept(t, true, "selftest")) | |||
| } | |||
|
|
|||
| // Cache terraform binary in the temporary directory to avoid downloading it in each test. | |||
There was a problem hiding this comment.
Thanks, this is indeed a needed feature, both for local tests but most importantly for cloud tests (#2242).
Could you make a separate PR just for cached terraform - it's important that we review it well.
Also, could you not depend on side effect of "bundle validate"? Here some scripts/docs on how to prepare terraform.
There was a problem hiding this comment.
I've seen instructions for air-gapped environments. I'm not sure I will have enough capacity to do it correctly. I think we need to implement it by relying on caching in GitHub actions and adding a step where we download Terraform. The cache key should be based on Terraform and Terraform provider versions. Right now, we don't even do that in our own GitHub action that we provide.
There was a problem hiding this comment.
I think Github actions cache is nice to have, not a must.
I'm not sure I will have enough capacity to do it correctly.
Let's discuss on Slack -- I can help with that.
There was a problem hiding this comment.
Please rebase on top of this #2267
|
|
||
| trace $CLI bundle deploy | ||
|
|
||
| # FIXME: doesn't work because fake state is shared and tests run in parallel |
There was a problem hiding this comment.
Please use Badness field in test.toml (git grep for examples).
There was a problem hiding this comment.
I think we should just fix the fake. It isn't hard
acceptance/server_test.go
Outdated
| }, nil | ||
| path := r.URL.Query().Get("path") | ||
|
|
||
| return server.getStatus(path) |
There was a problem hiding this comment.
why extra indirection with getStatus function? I don't see that helper used anywhere else.
Same comment for workspaceMkdirs, jobsCreate, jobsList - those are only used once, can be inlined.
There was a problem hiding this comment.
Can we have a separate PR for jobs API in server_test.go (excluding changes to python mutator and tests for python mutator)?
| mode: development | ||
| default: true | ||
| workspace: | ||
| host: $DATABRICKS_HOST |
There was a problem hiding this comment.
Why do we need this here? isn't that the default?
There was a problem hiding this comment.
Do you mean that databricks.yml doesn't have to declare targets if env variables are set? Note that we don't use "bundle init" here.
There was a problem hiding this comment.
I mean that $DATABRICK_HOST is already what's going to be used (that's how all other tests work), so no need to specify it explicitly and use .tmpl.
That line can just be deleted, it should work.
## Changes Fix relative path errors in the Python mutator that was failing during deployment since v0.239.1. Before that: ``` % databricks bundle deploy Deploying resources... Updating deployment state... Error: failed to compute relative path for job jobs_as_code_project_job: Rel: can't make resources/jobs_as_code_project_job.py relative to /Users/$USER/jobs_as_code_project ``` As a result, the bundle was deployed, but the deployment state wasn't updated. ## Tests Unit tests, adding acceptance tests in #2254
|
An authorized user can trigger integration tests manually by following the instructions below: Trigger: Inputs:
Checks will be approved automatically on success. |
## Changes - Add a script install_terraform.py that downloads terraform and provider and generates a config to use, inspired by https://gist.github.com/pietern/1cb6b6f3e0a452328e13cdc75031105e - Make acceptance tests run this script once before running the tests and set the required env vars to make cli use this terraform installation. - Use OS-specific directory for things that are build by acceptance test runner (CLI and terraform). This enables acceptance tests against cloud #2242 and local test for bundle deploy #2254. ## Tests - Add an acceptance test for standalone terraform. This is useful to debug terraform with TF_LOG=DEBUG to see that it uses local provider. - Other acceptance tests are updated with regard to terraform exec path. - The overall time for tests locally is unchanged (if terraform is already fetched).
Changes
Add acceptance tests for bundle deployment. Includes regression test for #2253
Terraform binary is required for deployment. We cache it once at the beginning of tests.
Because fake is stateful and shared between tests running in parallel, it isn't possible to inspect the state after the bundle is deployed. One option is to use different API tokens between tests and handle them as different workspaces. A different alternative is separating tests that rely on state from stateless tests.
Tests
Fake implementation is not tested but has good enough fidelity