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

Adding delete signaling channel API #346

Merged
merged 3 commits into from
Apr 14, 2020
Merged

Conversation

MushMal
Copy link
Contributor

@MushMal MushMal commented Apr 14, 2020

Adding tests and fixing some of the existing tests

Issue #328

Description of changes:

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Adding tests and fixing some of the existing tests
@MushMal MushMal requested review from Sean-Der and chehefen April 14, 2020 01:29
* Attempting to connect to the channel or send a message will result in an
* error or an unpredictable results after this call.
*
* NOTE: The call transitions the signaling client state machine to a terminal state
Copy link
Contributor

Choose a reason for hiding this comment

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

so what happens when signalingClientDeleteSync fails but the signaling channel is not successfully deleted? Is it still possible to delete it using the sdk?

Copy link
Contributor

Choose a reason for hiding this comment

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

when delete channel is called, can we just terminate the state machine and wait on delete api to be finished? The delete channel operation seems orthogonal to any of the state machine operations. This way in the state machine we dont need to worry if about delete happening.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The state is a terminal state. There isn't much left for the application to do but to either issue the delete call again or free the client.

We can't simply terminate the state machine as the delete needs the state machine for a couple of reasons - 1) in case of the stale credentials we will need to iterate to get token state and 2) in case of the delete failing with 400 we might have a bad version in which case we need to call describe.

@codecov-io
Copy link

Codecov Report

Merging #346 into master will decrease coverage by 0.04%.
The diff coverage is 83.48%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #346      +/-   ##
==========================================
- Coverage   87.03%   86.99%   -0.05%     
==========================================
  Files          33       33              
  Lines        8078     8173      +95     
==========================================
+ Hits         7031     7110      +79     
- Misses       1047     1063      +16     
Impacted Files Coverage Δ
src/source/Signaling/StateMachine.c 75.12% <73.52%> (-0.67%) ⬇️
src/source/Signaling/Client.c 100.00% <100.00%> (ø)
src/source/Signaling/LwsApiCalls.c 84.57% <100.00%> (-0.06%) ⬇️
src/source/Signaling/Signaling.c 95.29% <100.00%> (+0.39%) ⬆️
src/source/Ice/IceAgent.c 94.46% <0.00%> (+0.19%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b53b741...c351d4a. Read the comment docs.

@MushMal MushMal merged commit 909277d into master Apr 14, 2020
@MushMal MushMal deleted the delete_signaling_channel_api branch April 14, 2020 21:50
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