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

add add_on and remove_on fn to deprecate set_on_param fn #1134

Merged
merged 5 commits into from
May 26, 2020
Merged

add add_on and remove_on fn to deprecate set_on_param fn #1134

merged 5 commits into from
May 26, 2020

Conversation

claireyywang
Copy link
Contributor

Part of the effort to deprecate set_on_parameters_set_callback function in #1123 . It adds add_on_set_parameters_callback and remove_on_set_parameters_callback to lifecycle_node

@claireyywang
add add_on and remove_on
b3e526c 
Signed-off-by: claireyywang <22240514+claireyywang@users.noreply.github.com>
@claireyywang
add }
9de9c46 
Signed-off-by: claireyywang <22240514+claireyywang@users.noreply.github.com>
@claireyywang claireyywang added the in review Waiting for review (Kanban column) label May 26, 2020
@claireyywang claireyywang self-assigned this May 26, 2020
@claireyywang
Copy link
Contributor Author

claireyywang commented May 26, 2020 •
edited
Loading

CI on all ros2 packages
linux Build Status
linux-aarch64 Build Status
osx Build Status
windows Build Status

@claireyywang claireyywang mentioned this pull request May 26, 2020
Merged
jacobperron
jacobperron approved these changes May 26, 2020
View reviewed changes
Copy link
Member

@jacobperron jacobperron left a comment

Choose a reason for hiding this comment

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

LGTM pending CI

One minor comment.

rclcpp_lifecycle/src/lifecycle_node.cpp Outdated Show resolved Hide resolved
@claireyywang
remove return from void fn
63a48a1 
Signed-off-by: claireyywang <22240514+claireyywang@users.noreply.github.com>
@claireyywang
revert
e76a1bb 
Signed-off-by: claireyywang <22240514+claireyywang@users.noreply.github.com>
@claireyywang
reduce line length
223aeec 
Signed-off-by: claireyywang <22240514+claireyywang@users.noreply.github.com>
@claireyywang
Copy link
Contributor Author

CI up-to rclcpp_lifecycle
linux Build Status
linux-aarch64 Build Status
osx Build Status
windows Build Status

jacobperron
jacobperron approved these changes May 26, 2020
View reviewed changes
Copy link
Member

@jacobperron jacobperron left a comment

Choose a reason for hiding this comment

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

LGTM (looks like we have the same CI failures on our nightlies)

@claireyywang claireyywang merged commit 01ec06d into master May 26, 2020
@delete-merged-branch delete-merged-branch bot deleted the claire/add_missing_fn_lifecyle_node branch May 26, 2020 22:35
@ivanpauno
Copy link
Member

@claireyywang This seem to have introduced some failures on nightlies https://ci.ros2.org/view/nightly/job/nightly_linux_release/1555/#showFailuresLink.

Psst: We usually use squash merge instead of merge.

@ivanpauno ivanpauno mentioned this pull request May 27, 2020
Merged
@jacobperron
Copy link
Member

@ivanpauno Can you point to the specific failure? It looks like the same failures have been occurring since May 23: https://ci.ros2.org/view/nightly/job/nightly_linux_release/1551/

@ivanpauno
Copy link
Member

@ivanpauno Can you point to the specific failure? It looks like the same failures have been occurring since May 23: https://ci.ros2.org/view/nightly/job/nightly_linux_release/1551/

Yeap, sorry for the noise.
I will look for the actual root cause of the issue.

@Karsten1987 Karsten1987 mentioned this pull request May 27, 2020
Merged
@wjwwood
Copy link
Member

wjwwood commented May 27, 2020

Psst: We usually use squash merge instead of merge.

Yeah, please use squash in the future. It makes cherry-picking to different distributions so much easier. <3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wjwwood wjwwood wjwwood approved these changes

@jacobperron jacobperron jacobperron approved these changes

Assignees

@claireyywang claireyywang

Labels
in review Waiting for review (Kanban column)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@claireyywang @ivanpauno @jacobperron @wjwwood