-
Notifications
You must be signed in to change notification settings - Fork 43
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
Ensure all sdkv2 test providers have an id and an update method #2723
base: vvm/refactor_sdkv2_set_detailed_diff_tests
Are you sure you want to change the base?
Ensure all sdkv2 test providers have an id and an update method #2723
Conversation
This change is part of the following stack:
Change managed by git-spice. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## vvm/refactor_sdkv2_set_detailed_diff_tests #2723 +/- ##
===========================================================================
Coverage 69.63% 69.63%
===========================================================================
Files 301 301
Lines 38725 38725
===========================================================================
+ Hits 26965 26968 +3
+ Misses 10243 10241 -2
+ Partials 1517 1516 -1 ☔ View full report in Codecov by Sentry. |
1a68ff8
to
3a2e7a4
Compare
40369a1
to
a350c93
Compare
3a2e7a4
to
e04c35f
Compare
a350c93
to
2e69e46
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understand this change. We believe that resourceNeedsUpdate
is wrong, but are still using it?
Where does this cause us problems?
if resourceNeedsUpdate(r) && r.UpdateContext == nil { | ||
if r.UpdateContext == nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to add the if !resourceNeedsUpdate(r) { ... }
block if we remove the conditional.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds like EnsureProviderValid is actually doing something like populating defaults into the resource schemas. Adding update_prop is really non-intuitive to callers, why would it do that?
What if we just inlined and got rid of EnsureProviderValid and made the resource schemas in every test realistic, with comments e.g. if update_prop is added then why it's added?
This change tweaks the
EnsureProviderIsValid
method inpulcheck
to always give resources anid
property and ensure they pass the TF validation with respect to Update methods.Previously we'd run into trouble with TF validation when a resource needs an Update and doesn't have one or it doesn't need an Update and has one. The rules are non-trivial so I've opted to just make all resources need an Update and ensure they have one.