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

Ensure all sdkv2 test providers have an id and an update method #2723

Open
wants to merge 1 commit into
base: vvm/refactor_sdkv2_set_detailed_diff_tests
Choose a base branch
from

Conversation

VenelinMartinov
Copy link
Contributor

@VenelinMartinov VenelinMartinov commented Dec 11, 2024

This change tweaks the EnsureProviderIsValid method in pulcheck to always give resources an id 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.

Copy link

codecov bot commented Dec 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69.63%. Comparing base (e04c35f) to head (2e69e46).

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.
📢 Have feedback on the report? Share it here.

Copy link
Member

@iwahbe iwahbe left a 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?

Comment on lines -78 to +92
if resourceNeedsUpdate(r) && r.UpdateContext == nil {
if r.UpdateContext == nil {
Copy link
Member

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.

Copy link
Member

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?

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