Improve the external API of the PropertyBag

[theunrepentantgeek]

What this PR does / why we need it:

The original API of the property bag was going to be troublesome to generate code to consume, partly because of the way it combined checking for existence with deserialization. I've split those functions out into separate methods, which will allow code generation to avoid name collisions by using a nested scope:

if propertyBag.Contains("FullName") {
    var fullName string
    err := propertyBag.Pull("FullName", &fullName)
    if err != nil {
        return errors.NewError("failed to read property FullName from propertyBag")
    }

    destination.FullName = fullName
}

Special notes for your reviewer:

Split out as a separate PR to help keep the main PropertyBag usage PR smaller.

How does this PR make you feel:

If applicable:

• this PR contains tests

[codecov-commenter]

Codecov Report

Merging #1669 (1ed64b8) into master (e697bc1) will increase coverage by 0.08%.
The diff coverage is 72.72%.

@@            Coverage Diff             @@
##           master    #1669      +/-   ##
==========================================
+ Coverage   68.30%   68.39%   +0.08%     
==========================================
  Files         217      220       +3     
  Lines       15438    15581     +143     
==========================================
+ Hits        10545    10656     +111     
- Misses       4131     4155      +24     
- Partials      762      770       +8     

Impacted Files	Coverage Δ
hack/generated/pkg/genruntime/property_bag.go	84.84% <72.72%> (+8.98%)	⬆️
hack/generator/pkg/astmodel/property_injector.go	59.09% <0.00%> (-20.91%)	⬇️
...ted/pkg/reconcilers/azure_deployment_reconciler.go	68.80% <0.00%> (-0.96%)	⬇️
hack/generator/pkg/astmodel/types.go	67.51% <0.00%> (-0.46%)	⬇️
...ack/generated/pkg/genruntime/resource_reference.go	78.37% <0.00%> (ø)
.../generated/pkg/genruntime/conditions/conditions.go	51.16% <0.00%> (ø)
...g/testcases/object_type_serialization_test_case.go
...ator/pkg/codegen/pipeline/add_status_conditions.go	86.20% <0.00%> (ø)
...ator/pkg/testcases/json_serialization_test_case.go	2.22% <0.00%> (ø)
...ck/generator/pkg/astmodel/conditioner_interface.go	100.00% <0.00%> (ø)
... and 6 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e697bc1...1ed64b8. Read the comment docs.

[matthchr]

minor: This isn't in the code you actually changed, but I'm curious why string is called out here but not other primitive types?

Feels like you could probably get away without any special casing and just always use json.Marshal? I am assuming that json.Marshal(str) just returns str but I don't know for sure.

[theunrepentantgeek]

I don't recall - might have been to avoid the overhead of calling json.Marshal when not required, or it may have been to handle round trip formatting issues. I do remember it was deliberate though.

[matthchr]

Might be worth leaving a comment there as to the reason? Especially if there was some problem if it wasn't handled specially?

[theunrepentantgeek]

I have a laundry list of minor code-gardening things to do post codegen-alpha-0, I'll add this to that.

[matthchr]

You aren't asserting that the actual content is correct here, just that it has the key?
Shouldn't you also check to make sure that the key maps to the correct value?

Same comment applies below in a couple other tests, but won't repeat.

[matthchr]

Hmm I see in the other tests (that you already had) that you may have those cases covered already.

Maybe this just becomes minor: rather than ReturnsBagWithExpectedContent use ReturnsBagWithExpectedKeys to make it clear you only care about asserting on keys here?

[theunrepentantgeek]

Other tests check that the values stored can be recalled cleanly, so I'm only worried in this test whether the expected keys are put into the bag. I'll rename the tests as you suggest.

[matthchr]

Approved as the active comments I have are pretty minor, and/or are on sections of the code that actually weren't changed as part of the PR.

Feel free to merge once you've taken a look at them.