-
Notifications
You must be signed in to change notification settings - Fork 478
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
Databricks Sample - Terraform IaC for Azure Databricks and Asset Bundle Deployment via CI/CD #911
base: main
Are you sure you want to change the base?
Conversation
@DilmurodMak - one of the validations is failing, can you take a look? |
@ydaponte , The pipeline are templates, it requires databricks workspaces exist and its urls are set in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leaving some comments that need to be addressed before we can merge into main. There are some best practices and alignment with the overall repo that will need to be done as for example the creation of a devcontainer for the sample. Thanks for the great work so far!
|
||
### Pre-requisites | ||
- Clone the repository | ||
- Install Terraform CLI if not installed already [Terraform Installation](https://learn.hashicorp.com/tutorials/terraform/install-cli) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can add this pre-requirements to be installed when launching the devcontainer
single_tech_samples/databricks/databricks_terraform/Infra/README.md
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of the png, can you please commit the drawio.svg version file of this diagram? We are starting to create a standard in the repo for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These was originally images, I will try to recreate in draw.io if necessary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there an editable version of this diagram that can be commited instead?
Co-authored-by: Yennifer Santos <[email protected]>
Co-authored-by: Yennifer Santos <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't run the container - the req.txt file is missing. Can you upload it so I can further test? Thanks!
&& apt clean | ||
|
||
# Copy and install dev dependencies | ||
COPY requirements-dev.txt /tmp/requirements-dev.txt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The requirements-dev.txt file is missing. The devcontainer build is failing with this error: ERROR: failed to solve: failed to compute cache key: failed to calculate checksum of ref bfc31110-fef0-4a1d-8108-d3bf8d65de30::x6ufq090g3b3ngzwutfeemidk: "/requ
irements-dev.txt": not found
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ydaponte, I see it was missing. I just added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just leaving the comment as we discussed earlier today.
|
||
**NOTE** - *When **`adb-workspace`** module runs it creates databricks workspace, and by default it creates a metastore in the same region. Databricks allows only **ONE METASTORE** per region. **`metastore-and-users`** module deploys new metastore with our required configurations, but we have to delete existing metastore prior running the module* | ||
|
||
**NOTE** - *During script execution you will receive `Error: cannot create metastore: This account with id <Account_ID> has reached the limit for metastores in region <Region>` * error. This is because we have reached the limit of metastores in the region. To fix this, we need to delete existing metastore and re-run the script.* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we change the script to check if the metastore already exists? If it doesn't exist we create it but if exists we should reuse it. In the environment where I'm testing we do have already a metastore and I can't delete it. I can point to a different region to workaround the problem, but realistically if someone wants to try chances are that the metastore already exists most of the time.
Pull Request Overview
This PR updates and enhances the Databricks deployment process using Terraform and Asset Bundle Deployment via GitHub Actions. It simplifies deployment for multi environment deployment.
Key Highlights
single_tech_samples/databricks/databricks_terraform
generate-databricks-workflows.sh
.Testing Steps
The Sample code covers the deployment from sandbox to development environment.
main
.main
.