-
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
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 @@
## master #2723 +/- ##
==========================================
- Coverage 69.64% 69.64% -0.01%
==========================================
Files 301 301
Lines 38726 38726
==========================================
- Hits 26971 26969 -2
- Misses 10237 10240 +3
+ Partials 1518 1517 -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?
Yeah, that's a bit of a weird one. Turns out Terraform is not consistent with its own rules and some valid schemas can not pass validation. We call "test": {
Type: schema.TypeSet,
Optional: true,
ForceNew: true,
Computed: true,
Elem: &schema.Resource{
Schema: map[string]*schema.Schema{
"nested": {
Type: schema.TypeString,
Optional: true,
},
"computed": {
Type: schema.TypeString,
Optional: true,
Computed: true,
},
},
},
}, This schema has all its top-level properties as This behaviour is very non-intuitive and took me a while to figure out that TF Validation has a bug here. We could say that each person who writes a test is responsible for dealing with this and working around this but that seems harder. I've instead opted to work around the whole mess by adding an Optional |
Thanks for the explanation, this makes sense! At the very least this needs to be a comment I think on the "update_prop" - that is it's a no-op prop added to make InternalValidate pass. |
2e69e46
to
0f1206a
Compare
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.