I see what you mean about not modifying the test files. Avoiding that (annoying, probably manual) churn makes sense to me +1

In the long run we probably will want to update the test data to be consistent with the command output for clarity, but having backwards-compatibility for now will keep this PR simple and let us update tests & the TEST_ANCESTRY env var over time.

The one thing that needs to change: The goal is to replace Asset.Ancestry with Asset.Ancestors, rather than having both present. (Test data should be able to specify either one for backwards-compatibility.)

It could be easiest to have a testAsset that has both fields as an intermediate step? But I'm fine with any solution that achieves that goal.

Is it better to add a comment to state the field is deprecated rather than delete it?

Is it better to add a comment to state the field is deprecated rather than delete it?

Although this is an exported field, in general we don't actually expect people to interact directly with the fields on the Asset struct, so I think deleting it would probably be fine.

But I'd be fine with only marking it deprecated as long as we omit it from json marshalling - that would maybe be a simpler solution?

Although this is an exported field, in general we don't actually expect people to interact directly with the fields on the Asset struct, so I think deleting it would probably be fine.

https://github.com/GoogleCloudPlatform/config-validator/blob/87b4ae546420814b2a6766a1b4569278f5e1627e/pkg/asset/asset.go#L21
I just found the validator would not allow removing that field.

Hm, interesting - that would probably be a bug in config-validator. ValidateAsset is only called in one place and it is immediately followed by a call to SanitizeAncestryPath, which is a compatibility shim to make sure it works whether ancestors or ancestry path was provided.

My understanding is that config-validator supports ancestry path for historical reasons (i.e. supporting terraform-validator - other tools like cft-scorecard use CAI directly and would already have to support ancestors), so we should be able to not supply it.

-- looked into it --

It looks like cft-scorecard works around this problem by calling SanitizeAncestryPath before calling ReviewAsset. We could do the same thing I guess but it seems like we should probably just switch the order of the calls inside ReviewAsset to sanitize first and then validate - does that seem reasonable?

Done removing the ancestry path field, and in the validate part, add the SanitizeAncestryPath before ReviewAsset .