Fix project setting creation #202
Conversation
Merge from master to fork
|
@pnag90 I converted this to a draft PR. You should still get build results on commits. When its working and ready for review, set it back to ready for review :) |
|
I have added a few comments but obviously it is up to @jdamata if he agrees with them or not. |
| } | ||
| return true | ||
| } else if a["field_values"] != nil { | ||
| if a["field_values"] != nil { |
There was a problem hiding this comment.
What are we gaining by refactoring in this way?
I would suggest that it is far far far more likely that a setting is single-valued rather than multi-valued so shouldn't we check that first as the most likely case?
Aside from it being a micro-optimisation, it makes more sense IMO to handle the most common case as early as possible, which is what the original code was doing
There was a problem hiding this comment.
What was happening is that terraform loads the value from the setting property from the resource.
But even the field value was not being set in the tf script, the value at this point is an empty string instead of being null.
There was a problem hiding this comment.
so...couldn't you check if a["value"] != nil && a["value"] != "" first?
You would possibly also need to account for them all being empty - because settings are cleared out vs "is it value or values or field_values" that has a value - maybe none of them do
There was a problem hiding this comment.
I though about that, but I think that we cannot guarantee that no one will set the value as "" for a specific setting
|
@pnag90 One other comment - since all the tests were passing before and you have not added any new tests then this suggests that the condition you are trying to fix (Which is unclear to me TBH) is not covered by the tests so....a) how does anyone know you fixed it at all? and b) It seems like it would make sense to add one or more tests to cover the scenarion to prove that it is fixed - and if they are run against the current code, they would fail. Having green tests before your change and and the same tests still green afterwards suggests that either your change does nothing or that the existing tests are insufficient. I suspect the latter, in which case they are still insufficient! |
The tests were working for specific dummy examples of settings. |
Update branch with master release (v0.16.9)
Yes, I think what @tbutler-qontigo is rightly mentioning is that we would like to have those "real" use case settings that were failing for you to be added in. It would improve our test coverage posture and also confirm that your changes work for the issues you had. Of course sanitize any data you are using in your project Also apology for the super late review. I reviewed this part way a long time ago and never came back to finish reading up. Thanks @tbutler-qontigo for reviewing this :) |
|
Added tests for covering edition of a property setting |
project