Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Force top level status properties to be optional #1694

Merged
merged 2 commits into from
Aug 6, 2021

Conversation

matthchr
Copy link
Member

@matthchr matthchr commented Aug 5, 2021

What this PR does / why we need it:
There are times when Status doesn't exist, such as when an object is first created. There's a bit of an unwritten rule that:

  1. Status is not a pointer on the resource.
  2. All top level status properties have omitempty set.

This way when there isn't a status, there's not a bunch of extra ""'s returned.

This is especially important for some multi-write cases between spec and status, as ""'s returned can overwrite in-memory data before it has a chance to be sent via a client.Status().Update() call

How does this PR make you feel:
gif

@codecov-commenter
Copy link

codecov-commenter commented Aug 5, 2021

Codecov Report

Merging #1694 (436ac4a) into master (462f59f) will decrease coverage by 0.10%.
The diff coverage is 39.47%.

❗ Current head 436ac4a differs from pull request most recent head a3da71a. Consider uploading reports for the commit a3da71a to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1694      +/-   ##
==========================================
- Coverage   68.29%   68.18%   -0.11%     
==========================================
  Files         217      218       +1     
  Lines       15438    15474      +36     
==========================================
+ Hits        10543    10551       +8     
- Misses       4132     4154      +22     
- Partials      763      769       +6     
Impacted Files Coverage Δ
hack/generator/pkg/astmodel/types.go 67.51% <33.33%> (-0.46%) ⬇️
...odegen/pipeline/make_status_properties_optional.go 38.70% <38.70%> (ø)
hack/generator/pkg/codegen/code_generator.go 89.17% <100.00%> (+0.06%) ⬆️
hack/generator/pkg/astmodel/package_reference.go 88.28% <0.00%> (-2.71%) ⬇️
...ted/pkg/reconcilers/azure_deployment_reconciler.go 68.33% <0.00%> (-0.96%) ⬇️

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 462f59f...a3da71a. Read the comment docs.

Some services use the same status or spec for multiple resources. We
haven't noticed before because currently those types are pruned early.
@matthchr matthchr merged commit c4f0f09 into Azure:master Aug 6, 2021
@matthchr matthchr deleted the feature/optional-status branch August 6, 2021 17:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants