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.

Sign up for GitHub

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

Jump to bottom

Added parameter service_name to realm_app_call_function #6394

Merged
merged 9 commits into from
May 3, 2023
Merged

Added parameter service_name to realm_app_call_function #6394

merged 9 commits into from
May 3, 2023

Conversation

desistefanova
Copy link
Contributor

@desistefanova desistefanova commented Mar 17, 2023 •
edited
Loading

What, How & Why?

Added new parameter service_name to realm_app_call_function, which is exposed through realm.h. Having the service name would allow us executing the system functions for querying the Atlas app service documents.

☑️ ToDos

  • 📝 Changelog update
  • 🚦 Tests (or not relevant)
  • C-API, if public C++ API changed.

@cla-bot cla-bot bot added the cla: yes label Mar 17, 2023
@desistefanova desistefanova marked this pull request as ready for review March 17, 2023 10:01
@desistefanova desistefanova marked this pull request as draft March 17, 2023 10:02
@desistefanova desistefanova marked this pull request as ready for review March 17, 2023 10:27
fealebenpae
fealebenpae requested changes Mar 20, 2023
View reviewed changes
Copy link
Member

@fealebenpae fealebenpae left a 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 a @param doc entry for the new parameter in the header? Since it’s optional on the C++ side we need to let users of the API know it can be NULL and unfortunately we can’t do that using only C types.

nicola-cab
nicola-cab reviewed Mar 20, 2023
View reviewed changes
Copy link
Member

@nicola-cab nicola-cab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, provided CI is green. Also, this is a breaking change, will impact all the SDKs, but an entry has been added in the changelog.

@desistefanova
Copy link
Contributor Author

The feature that required this change has been postponed.

@desistefanova desistefanova marked this pull request as draft March 21, 2023 14:23
@desistefanova
Copy link
Contributor Author

I open this PR again, because we continue with the implementation of Atlas app services filter/CRUD/aggregate documents and we will need this change.

@desistefanova desistefanova marked this pull request as ready for review May 2, 2023 13:47
@nicola-cab nicola-cab self-requested a review May 3, 2023 14:35
@desistefanova desistefanova merged commit c9eb118 into master May 3, 2023
@desistefanova desistefanova deleted the ds/call_function_service_name_arg branch May 3, 2023 19:24
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@fealebenpae fealebenpae fealebenpae approved these changes

@nicola-cab nicola-cab nicola-cab approved these changes

@nirinchev nirinchev Awaiting requested review from nirinchev

@blagoev blagoev Awaiting requested review from blagoev

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@desistefanova @fealebenpae @nicola-cab