Uh oh!
There was an error while loading. Please reload this page.
Represent struct fields and map keys; fix ss_default to ignore map keys - #4041
Merged
Conversation
denik
requested review from
andrewnester, anton-107, pietern and shreyas-goenka
as code ownersDecember 1, 2025 13:46
Collaborator
Commit: 5204a5c
11 failing tests:
Top 26 slowest tests (at least 2 minutes):
|
denikforce-pushed
the
denik/remote-add-tag
branch
from
December 1, 2025 15:27
fce9d26 to
16e0aa4Comparedenik
changed the base branch from
main
to
denik/keyedslice-server-side-defaultDecember 1, 2025 15:30
| assert.Equal(t, map[string]any{ | ||
| `tags.env`: "test", | ||
| `tags.team`: "data", | ||
| `tags['env']`: "test", |
Contributor
There was a problem hiding this comment.
Add a case for when the map value is top level? And when the map key is deep nested?
ContributorAuthor
There was a problem hiding this comment.
added one test for top level map; deep nested is already tested here. In general, structwalk had only a minor update, so don't see a need for additional coverage. Unless you see something that can break?
| return p != nil && p.index == tagDotString | ||
| } | ||
| func (p *PathNode) DotString() (string, bool) { |
Contributor
There was a problem hiding this comment.
Are these methods used anywhere?
Contributor
There was a problem hiding this comment.
Would be nice to include a test to confirm they work.
Uh oh!
There was an error while loading. Please reload this page.
| // In any case, this is not a change we should react to. | ||
| // Note, we only consider StringKeys here, because indexes and key-value pairs refer to slices and we want to react to new element in slices. | ||
| // Note, IsStringKey is also too broad - it currently covers struct fields and map keys, we don't want to include map keys here. | ||
| // Note, we only consider struct fields here. Adding/removing elements to/from maps and slices should trigger updates. |
Contributor
There was a problem hiding this comment.
Note: Maps can have server-side defaults as well. Like tags for example.
Contributor
There was a problem hiding this comment.
Worth noting as a comment.
ContributorAuthor
There was a problem hiding this comment.
Can you elaborate? How does TF handle those?
Contributor
There was a problem hiding this comment.
For example, sql warehouses have a owner tag by default:
➜ cli-experimental-docker-tf-updated git:(experimental-docker-tf-updated) ✗ databricks warehouses get 0d8458ee2118a58e -p azure-ws | jq .tags
{
"custom_tags": [
{
"key": "Owner",
"value": "eng-dev-ecosystem-team_at_databricks.com"
}
]
}
Looks like Terraform does a suppress diff for these, i.e. ignoring remote changes: https://github.com/databricks/terraform-provider-databricks/blob/f5f476292a7e2198dcf4009d4c0c1411161637f6/sql/resource_sql_endpoint.go#L80
ContributorAuthor
There was a problem hiding this comment.
Thanks - good to know. If it's a fixed key we might be able to handle it more precisely than ignoring all custom_tags changes, but ignore custom_tags[key='Owner'] only.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
pietern
reviewed
Dec 1, 2025
| entry_point: "run" | ||
| environment_key: test_env | ||
| libraries: | ||
| - whl: hello.whl |
Contributor
There was a problem hiding this comment.
Curious, why the wheel if the test is about tags?
ContributorAuthor
There was a problem hiding this comment.
No reason, I just copied job config from another test.
Contributor
There was a problem hiding this comment.
Can be removed to keep the test tight. Since this is a local only test, the whole tasks block can be omitted.
denikforce-pushed
the
denik/remote-add-tag
branch
from
December 1, 2025 19:45
36ec8c3 to
af2b493Compare…e NewDotString/NewBracketString directly in some cases
shreyas-goenka
approved these changes
Dec 2, 2025
pietern
approved these changes
Dec 2, 2025
| } else { | ||
| // Map key with single quotes | ||
| } else if node.index == tagBracketString { | ||
| // Bracket notation: ['field'] |
Contributor
There was a problem hiding this comment.
Nice side-effect: formatting is no longer dependent on the node key.
| return p != nil && p.index == tagDotString | ||
| } | ||
| func (p *PathNode) DotString() (string, bool) { |
Contributor
There was a problem hiding this comment.
Would be nice to include a test to confirm they work.
| entry_point: "run" | ||
| environment_key: test_env | ||
| libraries: | ||
| - whl: hello.whl |
Contributor
There was a problem hiding this comment.
Can be removed to keep the test tight. Since this is a local only test, the whole tasks block can be omitted.
Uh oh!
There was an error while loading. Please reload this page.
eng-dev-ecosystem-bot
commented
Dec 2, 2025
Collaborator
Commit: 7783e65
47 failing tests:
Top 50 slowest tests (at least 2 minutes):
|
denik added a commit
that referenced
this pull request
May 20, 2026
…ys (#4041) ## Changes - Update structpath to distinguish between fields (.field) and map keys (["field"]). Note, when it comes to references, we still accept any syntax. However, structdiff and structwalk accurately represent map keys now. - Update server_side_default logic to ignore map keys. ## Why It was never the intention for map entries to have "server_side_default" feature. (Similar to slice indices in #4038). Issue appeared when we stopped distinguishing between fields and map keys in struct path #3640 Note, originally the difference between fields and map keys was removed so that a) users can use either syntax without worrying about matching the type. This is still the case, structaccess still treats those two the same way. b) "resources.jobs.foo" is printed with dots and not like 'resources.jobs["foo"]'. This is still the case, we don't use structpath's String() for this. ## Tests New regression test.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
Changes
Why
It was never the intention for map entries to have "server_side_default" feature. (Similar to slice indices in #4038).
Issue appeared when we stopped distinguishing between fields and map keys in struct path #3640
Note, originally the difference between fields and map keys was removed so that
a) users can use either syntax without worrying about matching the type. This is still the case, structaccess still treats those two the same way.
b) "resources.jobs.foo" is printed with dots and not like 'resources.jobs["foo"]'. This is still the case, we don't use structpath's String() for this.
Tests
New regression test.