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

Enable --no-daemon flag for some cli tools #514

Merged
merged 1 commit into from Jul 20, 2020

Conversation

dawonn-haval
Copy link
Contributor

fixes #511
added daemon support to the other topic tools, except pub.

Signed-off-by: Dereck Wonnacott dereck.wonnacott@havalus.com

@dawonn-haval
Copy link
Contributor Author

As far as I can tell, the unit tests that are failing run fine on my systems, and shouldn't have been affected by this commit.

Copy link
Contributor

@hidmic hidmic left a comment

Choose a reason for hiding this comment

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

@dawonn-haval I see three kinds of changes in this patch.

  1. NodeStrategy being used in CLI verbs such as delay
  2. Arguments specific to NodeStrategy being added where missing.
  3. Misc style changes.

I'd rather stick to (2.) only. (3.) changes are unrelated, and I'm not convinced (1.) is appropriate e.g. ros2 topic bw will always need a direct node to work.

@sloretz
Copy link
Contributor

sloretz commented May 28, 2020

@dawonn-haval friendly ping, what do you think of @hidmic's feedback?

@dawonn-haval
Copy link
Contributor Author

I'll push the requested changes momentarily.

@dawonn-haval dawonn-haval force-pushed the fix-no-daemon branch 2 times, most recently from 534962a to d8f8bc6 Compare May 28, 2020 01:42
@hidmic
Copy link
Contributor

hidmic commented Jun 18, 2020

CI up to ros2topic:

  • Linux Build Status

@hidmic
Copy link
Contributor

hidmic commented Jun 22, 2020

@dawonn-haval CI reports some linter errors, mind to take a look?

@dawonn-haval
Copy link
Contributor Author

Rebase Complete.

Build Status

Copy link
Contributor

@hidmic hidmic left a comment

Choose a reason for hiding this comment

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

LGTM! Running CI up to ros2topic one last time:

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

@hidmic
Copy link
Contributor

hidmic commented Jul 14, 2020

@dawonn-haval CI still reports linter issues.

@dawonn-haval
Copy link
Contributor Author

https://ci.ros2.org/job/ci_linux/11446/testReport/junit/ros2topic.ros2topic.test/test_flake8/test_flake8/

These don't show up on my local system or the pull request CI check; I did what the error told me to do and pushed the change, so I hope they are fixed now... Can you run that CI suite again please?

Fixes ros2#511

Signed-off-by: Dereck Wonnacott <dereck.wonnacott@havalus.com>
@dawonn-haval
Copy link
Contributor Author

Apparently didn't add the modified files to the commit... sorry, but can you roll it once more?

@hidmic
Copy link
Contributor

hidmic commented Jul 20, 2020

Alright, finally green. Thank you for your patience @dawonn-haval ! Merging.

@hidmic hidmic merged commit acb13d9 into ros2:master Jul 20, 2020
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.

--no-daemon is not valid
4 participants