fix Secret/secretmanager.aws custom diff logic when replica config is empty #1144
Conversation
… empty Signed-off-by: Erhan Cagirici <[email protected]>
|
/test-examples="examples/secretsmanager/v1beta1/secret.yaml" |
|
@erhancagirici this looks like a good fix, but in the interest of improving our test coverage, I'm puzzled why uptest doesn't fail on When I tried to reproduce the issue, I got the error described in #1128 when I was observing an existing secret, by setting the external name annotation, but not when I just created a new secret. It seems like that should be caught by the import step in uptest, yet that passes consistently for me. Any ideas why? |
|
It looks like the key difference in reproducing this bug is that when I duplicated the existing resource, I changed its metadata.name, which changed one of the crossplane-managed tags, which created a non-nil diff. This makes me want to get Update tests actually running in uptest for provider-aws. |
There was a problem hiding this comment.
I was able to reproduce the bug using uptest, and opened #1154 with examples that fail on main, but pass on this branch.
I'd be happy for you to just cherry-pick/copy in that commit to your branch, or merge my PR and rebase, or whatever other git workflow you find easiest.
| @@ -35,13 +35,25 @@ func Configure(p *config.Provider) { //nolint:gocyclo | |||
| return diff, nil | |||
| } | |||
|
|
|||
| resData, err := schema.InternalMap(r.TerraformResource.SchemaMap()).Data(state, diff) | |||
| if err != nil { | |||
| return nil, errors.New("could not construct resource data") | |||
There was a problem hiding this comment.
Is this really something we want to fail the whole observe loop for? I don't fully understand when this could happen, so I don't know how bad it is.
There was a problem hiding this comment.
under "normal" operation this should never return an error. An error here indicates an issue with the resource schema or the produced state. More specifically, this is a very fundamental statement and the same line is also run by the terraform SDK library already in earlier stages of observe, so any error should be already emitted if any before visiting the custom diff function.So, IMHO it is OK to fail the whole observe here
erhancagirici
requested review from
ulucinar,
sergenyalcin and
turkenf
as code owners
Many thanks 🎉
I was also just about to send a very similar example change, and for the sake of completeness of this PR, I will cherrypick your commit here. Thanks again for the valuable contribution! |
|
/test-examples="examples/secretsmanager/v1beta1/secret.yaml" |
|
/test-examples="examples/secretsmanager/v1beta1/secret-withreplica.yaml" |
erhancagirici
merged commit 81d43d0
into
crossplane-contrib:main
|
Backport failed for Please cherry-pick the changes locally. git fetch origin release-0.47
git worktree add -d .worktree/backport-1144-to-release-0.47 origin/release-0.47
cd .worktree/backport-1144-to-release-0.47
git checkout -b backport-1144-to-release-0.47
ancref=$(git merge-base 105ce93b8e3b103e5757e2cd9a1394a6e55a22f5 3df0d8de85c8beb87fcec918013f96c9fd088c97)
git cherry-pick -x $ancref..3df0d8de85c8beb87fcec918013f96c9fd088c97 |
Description of your changes
Fixes the custom diff logic (introduced at #1107) for
replicaattribute when it is not specified in the config and the diff does not involvereplicafield. The custom diff function is triggered for any update diff, and the current logic tries to read thereplicafield unconditionally. Whenreplicais not specified in config, the get operation fails the whole custom diff function, therefore the Observe operation.This change fixes the custom diff logic by:
replica. We only need to changereplicaattibute is not specified in the MR spec. This is already handled properly by the regular diff and needs no customization.Fixes #1128
I have:
make reviewable testto ensure this PR is ready for review.How has this code been tested
Tested with the spec at #1128