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

[dbus] add retain_active_session option for deactivate API #2537

Merged
merged 1 commit into from
Oct 22, 2024
Merged

[dbus] add retain_active_session option for deactivate API #2537

merged 1 commit into from
Oct 22, 2024

Conversation

mia1yang
Copy link
Contributor

No description provided.

@codecov Codecov
Copy link

codecov bot commented Oct 12, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 0% with 5 lines in your changes missing coverage. Please review.

Project coverage is 45.51%. Comparing base (2b41187) to head (fd41cd1).
Report is 834 commits behind head on main.

Files with missing lines Patch % Lines
src/dbus/server/dbus_thread_object_rcp.cpp 0.00% 5 Missing ⚠️
Additional details and impacted files 
@@             Coverage Diff             @@
##             main    #2537       +/-   ##
===========================================
- Coverage   55.77%   45.51%   -10.26%     
===========================================
  Files          87      102       +15     
  Lines        6890    12197     +5307     
  Branches        0      894      +894     
===========================================
+ Hits         3843     5552     +1709     
- Misses       3047     6343     +3296     
- Partials        0      302      +302

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bukepo
Copy link
Member

bukepo commented Oct 12, 2024

Should disconnect be the default behavior? Perhaps adding an option "retain-active-session"?

@wgtdkp
Copy link
Member

wgtdkp commented Oct 12, 2024

Should disconnect be the default behavior? Perhaps adding an option "retain-active-session"?

+1

@mia1yang
Copy link
Contributor Author

Should disconnect be the default behavior? Perhaps adding an option "retain-active-session"?

Change force disconnect to be the default logic now. Thanks~

@mia1yang mia1yang changed the title [dbus] add force option for deactivate API [dbus] add retain_active_session option for deactivate API Oct 12, 2024
@mia1yang mia1yang marked this pull request as ready for review October 12, 2024 07:25
librasungirl
librasungirl approved these changes Oct 12, 2024
View reviewed changes
src/dbus/server/introspect.xml Outdated Show resolved Hide resolved
@bukepo
Copy link
Member

bukepo commented Oct 12, 2024

missing test?

@mia1yang
Copy link
Contributor Author

missing test?

Test is in openthread branch, and it seems no depends-on support on otbr (only ot have depends-on of otbr).
Need to merge openthread change firstly to clear the CI failure.

@mia1yang mia1yang mentioned this pull request Oct 12, 2024
Merged
@mia1yang mia1yang force-pushed the epskc_deactivate_force branch 3 times, most recently from 64e4fee to 7b9b2c0 Compare October 12, 2024 09:45
@mia1yang mia1yang requested a review from sunytt October 12, 2024 10:07
sunytt
sunytt approved these changes Oct 14, 2024
View reviewed changes
src/dbus/server/introspect.xml Show resolved Hide resolved
@mia1yang mia1yang force-pushed the epskc_deactivate_force branch 2 times, most recently from 49ab85b to dda477d Compare October 15, 2024 16:26
@mia1yang mia1yang requested a review from wgtdkp October 16, 2024 05:16
@mia1yang mia1yang force-pushed the epskc_deactivate_force branch 7 times, most recently from 6b6f51d to eb589c2 Compare October 16, 2024 12:32
@mia1yang
Copy link
Contributor Author

mia1yang commented Oct 16, 2024 •
edited
Loading

The CI failure of this PR is due to Dbus API new option added, but the original unit test in OT has not added the new option. The CI failure should be clear after PR https://github.com/openthread/openthread/pull/10829 merged. Highlight error info here: "ERROR:dbus.connection:Unable to set arguments () according to signature 'b'; TypeError: More items found in D-Bus signature than in Python arguments"

@mia1yang mia1yang requested a review from jwhui October 17, 2024 23:39
@librasungirl librasungirl self-requested a review October 18, 2024 04:37
sunytt
sunytt approved these changes Oct 18, 2024
View reviewed changes
Copy link
Contributor

@sunytt sunytt left a comment

Choose a reason for hiding this comment

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

LGTM, thanks

src/dbus/server/dbus_thread_object_rcp.cpp Show resolved Hide resolved
Irving-cl
Irving-cl approved these changes Oct 21, 2024
View reviewed changes
Copy link
Contributor

@Irving-cl Irving-cl left a comment

Choose a reason for hiding this comment

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

LGTM 👍

One small suggestion: it would be great to write PR description. Probably with a one-sentence summary and some explanation about the background,

@jwhui jwhui merged commit 9506b2e into openthread:main Oct 22, 2024
32 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wgtdkp wgtdkp wgtdkp approved these changes

@bukepo bukepo bukepo approved these changes

@sunytt sunytt sunytt approved these changes

@Irving-cl Irving-cl Irving-cl approved these changes

@jwhui jwhui jwhui approved these changes

@librasungirl librasungirl librasungirl approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@mia1yang @bukepo @wgtdkp @sunytt @Irving-cl @jwhui @librasungirl @yuzhyang