[Fix] Fixed syncing of effective fields in plugin framework implementation of share resource #4969
Conversation
tanmay-db
requested review from
renaudhartert-db
and removed request for
a team
tanmay-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
tanmay-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
tanmay-db
changed the title
tanmay-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
tanmay-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
tanmay-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
tanmay-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
tanmay-db
requested review from
mgyucht,
rauchy,
renaudhartert-db and
hectorcast-db
| for j := range planObjects { | ||
| if stateObjects[i].Name == planObjects[j].Name { | ||
| mode.objectLevel(ctx, &stateObjects[i], planObjects[j]) | ||
| finalObjects = append(finalObjects, stateObjects[i]) |
There was a problem hiding this comment.
what happens if the plan as an object which is not in the state? It seems that we are ignoring it.
There was a problem hiding this comment.
Yes, we ignore it because in terraform we want to create the infra from the config
There was a problem hiding this comment.
Can you explain a bit more what is what? I think I am mixing what stateObjects, planObjects and finalObjects are.
As it is, it feels that we are storing (state) a different set of objects that we are creating (plan).
| var d diag.Diagnostics | ||
| mode.resourceLevel(ctx, &state, plan.ShareInfo_SdkV2) | ||
| planObjects, _ := plan.GetObjects(ctx) | ||
| stateObjects, _ := state.GetObjects(ctx) |
There was a problem hiding this comment.
Are we suppressing the error? Can this fail?
There was a problem hiding this comment.
if d.HasError() {
panic(pluginfwcommon.DiagToString(d))
}The error isn't suppressed: https://github.com/databricks/terraform-provider-databricks/blob/main/internal/service/sharing_tf/legacy_model.go#L4440.
This was added in: https://github.com/databricks/terraform-provider-databricks/pull/4332/files#diff-5f47d9d2d5e2fcf6b377c528600d81c37f4526da6ce6aba43818bdb0f7039152R2365-R2375
rauchy
force-pushed
the
panic-share-pluginfw
branch
from
006c174 to
881c81c
Compare
…ricks into panic-share-pluginfw
tanmay-db
force-pushed
the
panic-share-pluginfw
branch
from
881c81c to
55e1a33
Compare
tanmay-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
tanmay-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
tanmay-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
tanmay-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
|
If integration tests don't run automatically, an authorized user can run them manually by following the instructions below: Trigger: Inputs:
Checks will be approved automatically on success. |
There was a problem hiding this comment.
Thanks for this @tanmay-db!
I think we discussed offline that you would add an integration test for this case as well, if I'm not mistaken: create the share, make out-of-band changes to add/remove shares, and then apply again and see that it is restored to the correct state. Or did you do manual testing of this?
| existingStateObjects, _ := existingState.GetObjects(ctx) | ||
| newStateObjects, _ := newState.GetObjects(ctx) |
There was a problem hiding this comment.
Let's not drop these errors here. I know this was like this before, but let's try to eliminate this tech debt.
There was a problem hiding this comment.
We aren't dropping the errors: https://github.com/databricks/terraform-provider-databricks/blob/main/internal/service/sharing_tf/legacy_model.go#L4440
if d.HasError() {
panic(pluginfwcommon.DiagToString(d))
}| break | ||
| } | ||
| } | ||
| finalObjects = append(finalObjects, newStateObjects[i]) |
There was a problem hiding this comment.
Just to clarify, the order is the server-returned order. What happens if the order of shares in the config is changed?
|
Yes, I plan to add the tests but didn't get to them yet as was working on other things, will send the PR in review once they are added. |
Changes
Planned objects can be less than the state objects. This can happen when someone removes an object in share resource outside the terraform (for example through UI). The changes fixes the syncing of effective fields by making sure we iterate over the planned and state objects properly.
Ref: #4913
Tests
Unit tests