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

Option to search for controller manager in the same namespace as spawner #810

Merged
merged 5 commits into from
Sep 28, 2022
Merged

Option to search for controller manager in the same namespace as spawner #810

merged 5 commits into from
Sep 28, 2022

Conversation

tonynajjar
Copy link
Contributor

@tonynajjar tonynajjar commented Sep 15, 2022

While it is possible to specify the controller manager's name including a namespace with the --controller-manager option, searching for the controller manager in the same namespace as the spawner makes my launch files for multi-robot simulation much simpler. That way I don't need to pass the namespace as a launch argument.

P.S: Happy to spend more time fixing CI and writing a test if the idea is approved

@tonynajjar tonynajjar changed the title Make controller manager use same namespace as spawner Option to search for controller manager in the same namespace as spawner Sep 15, 2022
@bmagyar
Copy link
Member

bmagyar commented Sep 22, 2022

I really like the idea, in fact so much I'd prefer it to be the default behaviour!
What do you think @destogl?

@tonynajjar
Copy link
Contributor Author

tonynajjar commented Sep 23, 2022

Nice! I do think it makes sense to be the default. One could still specify the controller name without a namespace if needed

Let me know if I should go forward with it

@destogl
Copy link
Member

destogl commented Sep 24, 2022

Let me know if I should go forward with it

Yes, this should be default behavior. Now sure if we need than this flag you introduced or something else (different name).

@tonynajjar
Copy link
Contributor Author

Now sure if we need than this flag you introduced or something else (different name).

If that's the default behavior, then I don't think we need any flag. Will submit some changes in a bit

@tonynajjar
Copy link
Contributor Author

Ready for review again.

The logic is that the spawner's namespace is added to the controller manager's name only if:

  1. The namespace exists (is not "/")
  2. the controller manager's name is not fully resolved (starts with "/")

I also changed the default for the controller manager's name to not be fully resolved

@bmagyar
Copy link
Member

bmagyar commented Sep 28, 2022

Thanks very much, let's roll it!

@bmagyar bmagyar merged commit 1088ac2 into ros-controls:master Sep 28, 2022
@destogl
Copy link
Member

destogl commented Oct 15, 2022

@Mergifyio backport humble

@mergify
Copy link
Contributor

mergify bot commented Oct 15, 2022

backport humble

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request Oct 15, 2022
destogl pushed a commit that referenced this pull request Oct 15, 2022
destogl pushed a commit that referenced this pull request Oct 15, 2022
…#839)

(cherry picked from commit 1088ac2)

Co-authored-by: Tony Najjar <tony.najjar.1997@gmail.com>
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

3 participants