Skip to content

add logging permission profile #80

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
Merged
ruffsl merged 3 commits into from
Feb 21, 2019
Merged

add logging permission profile #80

ruffsl merged 3 commits into from
Feb 21, 2019

Conversation

@mikaelarguedas
Copy link
Member

@mikaelarguedas mikaelarguedas commented Feb 18, 2019

This PR targets #72, not master

Since ros2/rcl#350 all node need publish access to /rosout by default.

I called it logging in case we have other related default logging topics in the future.

@ruffsl FYI

@mikaelarguedas mikaelarguedas added in progress Actively being worked on (Kanban column) in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Feb 18, 2019
@ruffsl
Copy link
Member

ruffsl commented Feb 18, 2019

Ah man... its like ROS1 all over again. Why is everything is needing to publish to root level topics and parameters by default.

The test case may need to be updated though:

https://github.com/ros2/sros2/pull/72/files#diff-9534083787043b52ef6d287331dfc69bR53

ruffsl
ruffsl reviewed Feb 18, 2019
View reviewed changes
@mikaelarguedas mikaelarguedas mentioned this pull request Feb 18, 2019
Closed
@mikaelarguedas
Copy link
Member Author

mikaelarguedas commented Feb 18, 2019

The test case may need to be updated though:

Good point 👍, test updated in e385cbd

This was referenced Feb 18, 2019
Closed
Closed
@mikaelarguedas
make rosout relative
f2061f1 
Signed-off-by: Mikael Arguedas <[email protected]>
ruffsl
ruffsl approved these changes Feb 21, 2019
View reviewed changes
Copy link
Member

@ruffsl ruffsl left a comment

Choose a reason for hiding this comment

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

LGTM

@jacobperron
Copy link
Member

jacobperron commented Feb 21, 2019 •
edited
Loading

Ignore unrelated test failures in rclcpp_action and test_cli:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

Edit: Retriggering with ros2/ci#246 (which I think is required)

@jacobperron
Copy link
Member

jacobperron commented Feb 21, 2019

Looks like the changes in ros2/ci#246 need to be rebased on master, I'll just wait until it is merged before triggering CI.
@ruffsl I think you can go ahead and merge this into your branch if you're happy with it.

@ruffsl ruffsl merged commit 4e7c398 into ros2:xml_profile Feb 21, 2019
@ruffsl ruffsl removed the in review Waiting for review (Kanban column) label Feb 21, 2019
@mikaelarguedas mikaelarguedas deleted the logging_xml_profile branch February 21, 2019 20:13
@ruffsl ruffsl mentioned this pull request Feb 26, 2019
Merged
@mjcarroll mjcarroll mentioned this pull request Mar 11, 2019
Merged
@nuclearsandwich nuclearsandwich mentioned this pull request Mar 13, 2019
Closed
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jacobperron jacobperron jacobperron left review comments

@ruffsl ruffsl ruffsl 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.

3 participants

@mikaelarguedas @ruffsl @jacobperron