Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Added support for Databricks Apps in DABs #1928
base: main
Are you sure you want to change the base?
Added support for Databricks Apps in DABs #1928
Changes from 22 commits
e9d9caf
395645f
f51eefe
483a239
ddc012c
28d69a5
1a58f1b
2c359b4
a71289a
ddeeefe
1ff7bd2
53393d5
e7a377c
6655eb6
1ceec5e
e8cae54
ab85a7f
f6de4c1
e9e0566
e622ab5
db516b6
2f8df81
0583ded
86630b6
4b4cc42
7693f0a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
This test works but doesn't actually use the translation map.
I suspect you intend to make sure that the conversion of bundle-internal field references to Terraform resource field references is undone so that we can interpolate the IDs ourselves. But the test runs against a bundle-internal field reference.
Is there an alternative approach such that we don't have to convert back-and-forth? Maybe we completely skip this path during variable resolution? Or only skip
resources.*
references underresources.apps.*.config.**
? Then we have less coupling with the TF resource interpolation.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 prefer to keep conversion to TF for all resources and then having this separate mutator which converts it back. The reasons are:
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.
That works for me.
The test now uses the TF notation, so it will fail when that is changed.
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.
@fjakobs @ilyakuz-db FYI, this is a potential problem for source-linked deployments. We assume here that files are deployed under
${workspace.file_path}
and then write a file into it. If we were to make this work for source-linked deployments, this would have to write a file to the source folder itself, which in turn means we'll get validation errors (we check that the user does not have both anapp.yml
and a configuration section in the bundle configuration for their app.The fact that we have to think about this (and potentially work around it) illustrates the problem that this duality brings.
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.
To confirm, you're using Go() because Write() could be an API call?
This feels like a race condition - are you sure you cannot lose diag.Diagnostic there?
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.
Right, good catch! Missed it during refactoring to parallel calls.