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

Remodel ros2controlcli #349

Merged
merged 35 commits into from
Mar 30, 2021
Merged

Remodel ros2controlcli #349

merged 35 commits into from
Mar 30, 2021

Conversation

Karsten1987
Copy link
Contributor

This currently sits on top of #310 but also addresses #341.

The current PR of @v-lopez does unfortunately not pass all its tests on OSX and I've noticed that the CLI package was in a rather wild state. This PR is also addressing their linters and refactors a lot of the node handling.
It also activates the tests of this package in the GH actions.

Victor Lopez and others added 21 commits March 23, 2021 08:07
Signed-off-by: Victor Lopez <victor.lopez@pal-robotics.com>
Signed-off-by: Victor Lopez <victor.lopez@pal-robotics.com>
Signed-off-by: Victor Lopez <victor.lopez@pal-robotics.com>
fixes #320

Signed-off-by: Victor Lopez <victor.lopez@pal-robotics.com>
Co-authored-by: Denis Štogl <destogl@users.noreply.github.com>
Co-authored-by: Karsten Knese <Karsten1987@users.noreply.github.com>
Signed-off-by: Karsten Knese <Karsten1987@users.noreply.github.com>
Signed-off-by: Karsten Knese <Karsten1987@users.noreply.github.com>
Signed-off-by: Karsten Knese <Karsten1987@users.noreply.github.com>
Signed-off-by: Karsten Knese <Karsten1987@users.noreply.github.com>
@Karsten1987
Copy link
Contributor Author

I will still have to modify the spawner and unspawner scripts to use the python api within the controller manager to get rid of the python subprocess calls.

@bmagyar
Copy link
Member

bmagyar commented Mar 28, 2021

merged #310, rebased this one

Copy link
Member

@bmagyar bmagyar left a comment

Choose a reason for hiding this comment

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

Good start but I think this PR needs the removal of those subprocess calls too to be complete.
Thanks for adding the gh actions, good catch!

Please always elaborate on OSX issues you find as you are the only source of this info in our group. It'd be nice if you could actually open an issue with them whenever you find some and mark PRs that resolve them, that way we know they are solved for everyone.

Signed-off-by: Karsten Knese <Karsten1987@users.noreply.github.com>
Signed-off-by: Karsten Knese <Karsten1987@users.noreply.github.com>
Signed-off-by: Karsten Knese <Karsten1987@users.noreply.github.com>
Signed-off-by: Karsten Knese <Karsten1987@users.noreply.github.com>
Signed-off-by: Karsten Knese <Karsten1987@users.noreply.github.com>
Signed-off-by: Karsten Knese <Karsten1987@users.noreply.github.com>
Signed-off-by: Karsten Knese <Karsten1987@users.noreply.github.com>
Signed-off-by: Karsten Knese <Karsten1987@users.noreply.github.com>
Signed-off-by: Karsten Knese <Karsten1987@users.noreply.github.com>
Signed-off-by: Karsten Knese <Karsten1987@users.noreply.github.com>
@Karsten1987
Copy link
Contributor Author

So I ended up changing the spawner scripts as well and with these changes the tests seem to work again.

I do believe there's a fine difference in the pep257 between the ros-tooling and ros-industrial CI. The source build (ros-tooling) seems to use a version which expects a semicolon ; after a doc section, whereas the binary jobs (ros-industrial) don't.
Not sure what to do here.

Signed-off-by: Karsten Knese <Karsten1987@users.noreply.github.com>
@@ -31,8 +30,8 @@ def add_arguments(self, parser, cli_name):
add_controller_mgr_parsers(parser)

def main(self, *, args):
print("deprecated warning: Please use either 'load --state' or 'set_state'")
Copy link
Member

Choose a reason for hiding this comment

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

set_state should be set_controller_state

@destogl
Copy link
Member

destogl commented Mar 31, 2021

@Karsten1987 great job! Thanks for doing this!

(I just opened a few follow-ups to clean the code even more)

@@ -31,8 +30,8 @@ def add_arguments(self, parser, cli_name):
add_controller_mgr_parsers(parser)

def main(self, *, args):
print("deprecated warning: Please use either 'load --state' or 'set_state'")
Copy link
Member

Choose a reason for hiding this comment

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

Remove this in galactic+

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.

None yet

4 participants