Closed ChihweiLHBird closed 6 months ago
Hi @ChihweiLHBird 👋
Thank you for this submission.
Could I ask that you add test coverage to verify the behaviour of this change?
Perhaps a new test to confirm that both the attribute upon which the AtLeastOneOf
validator is declared, and all of the attributes supplied in the path.Expression
args appear in the diagnostic.
Could you also add a change log entry as described in the New Pull Request section of the doc on contributing?
Thanks again.
Hi @bendbennett, thanks for letting me know! I just added them.
Hi @bflad, thanks for the suggestion! I think this suggestion is fine, but one downside is that in the future, if any error message is changed in the validator, the tests will need to be adjusted accordingly. This is somehow against the design principle of SSOT and might lead to more effort of maintenance required in the future IMO. However, they are only few error messages, thus the impact is pretty minimum and I think it would be fine. If you think it's okay, I can proceed to this way.
@ChihweiLHBird thank you very much for your PR. This looks good to me.
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions. If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.
In the schema:
Let's don't put both attributes in the Terraform config file.
Previous error message:
This message above is misleading because it actually requires one of
attribute1
andattribute2
, butattribute1
is not in the error message.Now with the change in this PR:
We can correctly inform the user what attributes are required.