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 file_name_prefix parameter to external log configuration. #109

Merged
merged 3 commits into from
Jan 2, 2024
Merged

add file_name_prefix parameter to external log configuration. #109

merged 3 commits into from
Jan 2, 2024

Conversation

fujitatomoya
Copy link
Collaborator

@fujitatomoya
add file_name_prefix parameter to external log configuration.
737b943 
Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
This was referenced Dec 13, 2023
Merged
Closed
@fujitatomoya
Copy link
Collaborator Author

@iuhilnehc-ynos requesting review.

iuhilnehc-ynos
iuhilnehc-ynos reviewed Dec 14, 2023
View reviewed changes
Copy link

@iuhilnehc-ynos iuhilnehc-ynos left a comment

Choose a reason for hiding this comment

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

a minior issue, otherwise LGTM.

rcl_logging_spdlog/src/rcl_logging_spdlog.cpp Outdated Show resolved Hide resolved
@fujitatomoya
add tests.
6e1a531 
Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
@fujitatomoya
Copy link
Collaborator Author

@iuhilnehc-ynos requesting 2nd review.

@fujitatomoya
Copy link
Collaborator Author

CI:

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

clalancette
clalancette requested changes Dec 20, 2023
View reviewed changes
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

Besides what I left inline, Windows CI doesn't seem too happy with this change. We'll also need to fix that.

rcl_logging_spdlog/src/rcl_logging_spdlog.cpp Outdated Show resolved Hide resolved
rcl_logging_spdlog/test/fixtures.hpp Outdated Show resolved Hide resolved
@fujitatomoya
address review comments.
1c6dffc 
Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
@fujitatomoya
Copy link
Collaborator Author

CI:

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

@fujitatomoya
Copy link
Collaborator Author

@clalancette requesting review again, windows failure has been addressed and CI is green.

this needs to go with ros2/rcl#1127

@clalancette clalancette merged commit fd629cf into ros2:rolling Jan 2, 2024
3 checks passed
@ZhenshengLee
Copy link

Happy to see the feature, I wonder if this feature will be applied to LTS Humble?

@fujitatomoya
Copy link
Collaborator Author

i do not think we can do that, this breaks API.

@ZhenshengLee
Copy link

i do not think we can do that, this breaks API.

Thanks for your reply! Which official version will this feature be applied/released to? Iron or the future J version?

@fujitatomoya
Copy link
Collaborator Author

Jazzy Jalisco, see distributions for https://docs.ros.org/en/rolling/Releases.html#future-distributions.

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

@iuhilnehc-ynos iuhilnehc-ynos iuhilnehc-ynos approved these changes

@clalancette clalancette clalancette approved these changes

@wjwwood wjwwood Awaiting requested review from wjwwood wjwwood is a code owner

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

Successfully merging this pull request may close these issues.

None yet
4 participants
@fujitatomoya @ZhenshengLee @clalancette @iuhilnehc-ynos