-
Notifications
You must be signed in to change notification settings - Fork 4
Add support for empty input lists #68
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
Conversation
ytsarev
left a comment
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.
@tvandinther thanks a lot for the PR. It is useful! Could you please add unit tests that cover the logic?
The current test suite mocks the whole query function with an implementation which performs the same check as what I have modified in the real implementation. https://github.com/upbound/function-msgraph/blob/main/fn_test.go#L374 Unsure how to proceed exactly since these changes sit below the mocked interface. I considered exiting early but that is technically a different implementation as the target should continue to have (empty) results written and status should continue as usual, rather than the query being skipped altogether. |
|
For now I've updated the mocks to have the same changes as in the real implementation to show that these changes work as intended, even though the code is not in the test path. |
f5a6033 to
6645d86
Compare
With this change, if no groups are queried, an empty list is returned instead of null. Signed-off-by: Tom van Dinther <39470469+tvandinther@users.noreply.github.com>
Also updates the mock to contain the same logic as added in the real implementation. Signed-off-by: Tom van Dinther <39470469+tvandinther@users.noreply.github.com>
Signed-off-by: Tom van Dinther <39470469+tvandinther@users.noreply.github.com>
380ff9e to
c0db281
Compare
ytsarev
left a comment
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.
Can you please add example to README and document new configuration field in https://github.com/upbound/function-msgraph?tab=readme-ov-file#input-configuration-options ? Otherwise, I believe it is good to merge 👍
Signed-off-by: Tom van Dinther <39470469+tvandinther@users.noreply.github.com>
|
Thanks @ytsarev, README update has been pushed. |
ytsarev
left a comment
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.
Awesome! @tvandinther thank you so much for the contribution!
Description of your changes
With the changes in this MR, the function now completes successfully with empty input lists or refs which resolve to empty lists.
Added a new optional boolean field on the function input named
failOnEmpty. This field opts into the previous behaviour of this function in case it is desired.Fixes #67
I have: