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

[Feature] Add databricks_app resource #4099

Open
wants to merge 25 commits into
base: main
Choose a base branch
from
Open

[Feature] Add databricks_app resource #4099

wants to merge 25 commits into from

Conversation

nkvuong
Copy link
Contributor

@nkvuong nkvuong commented Oct 12, 2024

Changes

  • Added databricks_app resource

Resolves #4084

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

@nkvuong nkvuong marked this pull request as ready for review October 29, 2024 09:48
@nkvuong nkvuong requested review from a team as code owners October 29, 2024 09:48
@nkvuong nkvuong requested review from mgyucht and removed request for a team October 29, 2024 09:48
Copy link
Contributor

@alexott alexott left a comment

Choose a reason for hiding this comment

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

small changes in the doc required, otherwise code looks good

apps/resource_app.go Outdated Show resolved Hide resolved
docs/resources/app.md Outdated Show resolved Hide resolved
docs/resources/app.md Outdated Show resolved Hide resolved
return err
}
// now deploy the app, using the source code path
createAppDeployment.AppName = app.Name
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we discussed it offline and agreed that the deployment won't be part of TF configuration. Instead, if an users want to deploy, they use CLI to do that. Otherwise it's confusing because we do the deployments only when source_code_path changed but we need to do deployments when source code is changed.

@pietern wdyt?

if err != nil {
return err
}
if d.HasChanges("source_code_path", "mode") {
Copy link
Contributor

Choose a reason for hiding this comment

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

See my comment above, I think we should remove app deployment altogether for now

@andrewnester
Copy link
Contributor

Just to clarify, I mean app resource needs to be added here so it can be part of the schema and be able to use it by DABs since we rely on generic databricks_permission resource anyway
https://github.com/databricks/terraform-provider-databricks/blob/main/permissions/permission_definitions.go#L346

@nkvuong
Copy link
Contributor Author

nkvuong commented Nov 22, 2024

@andrewnester makes sense - I've added apps to the permission resource and corresponding tests

Copy link
Contributor

@mgyucht mgyucht left a comment

Choose a reason for hiding this comment

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

Behavior-wise, this seems OK to me. Can we use the plugin framework to implement this instead of the SDKv2? We're trying to move away from SDkv2 to plugin framework for all new resources.

Comment on lines 37 to 39
appNameValidationFunc := validation.StringMatch(
regexp.MustCompile("^[a-z-]{2,30}$"),
"name must contain only lowercase alphanumeric characters and hyphens, and be between 2 and 30 characters long")
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's leave validation out and let server perform validation instead.

"update_time", "updater", "url"} {
s.SchemaPath(p).SetComputed()
}
return s
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's at least do the validation in the create/update. (Or at least test to verify what its behavior is.)

if err != nil {
return err
}
_, err = w.Apps.Update(ctx, apps.UpdateAppRequest{
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to wait for state (like in create)?

}
return err
}
d.SetId(app.Name)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we update this to get it back into a healthy state? if not this is fine. If so, let's set the ID before waiting for ready, then subsequent applies can call update.

)

var appAliasMap = map[string]string{
"resources": "resource",
Copy link
Contributor

Choose a reason for hiding this comment

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

@rauchy Can you discuss with @nkvuong about adding this alias to our API spec?

}
}

func ResourceApp() common.Resource {
Copy link
Contributor

Choose a reason for hiding this comment

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

BTW: Can we use the plugin framework for this new resource? Generally speaking, new resources should use the plugin framework, since it should be much more consistent for handling things like effective fields, dealing with lists, etc.


-> This feature is in [Public Preview](https://docs.databricks.com/release-notes/release-types.html).

Apps run directly on a customer’s Databricks instance, integrate with their data, use and extend Databricks services, and enable users to interact through single sign-on.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we link to public docs about Apps?


-> This feature is in [Public Preview](https://docs.databricks.com/release-notes/release-types.html).

Apps run directly on a customer’s Databricks instance, integrate with their data, use and extend Databricks services, and enable users to interact through single sign-on.
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably mention that this only creates the application and doesn't do anything with the app deployment.

Copy link

If integration tests don't run automatically, an authorized user can run them manually by following the instructions below:

Trigger:
go/deco-tests-run/terraform

Inputs:

  • PR number: 4099
  • Commit SHA: 2bac7aa7cc25a9ad8aebc939fe3ef965cdf6e84b

Checks will be approved automatically on success.

@eng-dev-ecosystem-bot
Copy link
Collaborator

Test Details: go/deco-tests/12044847955

@alexott alexott requested a review from mgyucht November 27, 2024 11:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEATURE] Add support for Databricks Apps
6 participants