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

Stop exposing ros-master port and better health check #148

Merged
merged 3 commits into from
May 23, 2022

Conversation

MithunKinarullathil
Copy link
Collaborator

@MithunKinarullathil MithunKinarullathil commented May 12, 2022

@xsoulp FYI. This should solve a bit of the issue you were mentioning.
@diasdm This should be enough for the sonarqube test?

  • Make sure you are opening from a topic/feature/bugfix branch
  • Ensure that the PR title represents the desired changes
  • Ensure that the PR description detail the desired changes
  • Link to relevant issues in GitHub or Jira
  • Link to relevant pull requests, esp. upstream and downstream changes
  • Ensure you have provided tests - that demonstrates feature works or fixes the issue

…ore outside

Better healthcheck for roscore container
@guide-bot
Copy link

guide-bot bot commented May 12, 2022

Thanks for opening this Pull Request!
We need you to:

  1. Complete the activities.

    Action: Complete Link to relevant issues in GitHub or Jira
    Action: Complete Link to relevant pull requests, esp. upstream and downstream changes
    Action: Complete Ensure you have provided tests - that demonstrates feature works or fixes the issue

    If an activity is not applicable, use '~activity description~' to mark it not applicable.

Copy link
Contributor

@AlexFernandes-MOVAI AlexFernandes-MOVAI left a comment

Choose a reason for hiding this comment

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

Can you please indicate the use cases list you want to achieve with this PR ?

  • better health check is one thing internal to the container : no problem to enhance it
  • not exposing the port of ros-master is something more functional and feature oriented, I am afraid this change is not enough to reach the aimed feature

@MithunKinarullathil
Copy link
Collaborator Author

This PR is just to close up a wound that is restricting the users without additional advantage. We have another good way to expose the ROS topics using the movai-cheatsheet, which has been tested thoroughly in the EE. It is better than exposing the full network to the host (which can have unintended consequences). So to summarize what this PR archives.

  • User is able to run their own ROS launches and ROS things in their host independently with movi-flow running beside it. (currently, this is not possible if movai-flow is running, which is restricting function without providing additional value)

  • For the short term we will document the steps from the cheat sheet, and how to expose topics manually to the community

  • For the long term, once the cheatsheet tools are exposed, they will be able to do this easily using the cli.

@AlexFernandes-MOVAI
Copy link
Contributor

This PR is just to close up a wound that is restricting the users without additional advantage. We have another good way to expose the ROS topics using the movai-cheatsheet, which has been tested thoroughly in the EE. It is better than exposing the full network to the host (which can have unintended consequences). So to summarize what this PR archives.

  • User is able to run their own ROS launches and ROS things in their host independently with movi-flow running beside it. (currently, this is not possible if movai-flow is running, which is restricting function without providing additional value)
  • For the short term we will document the steps from the cheat sheet, and how to expose topics manually to the community
  • For the long term, once the cheatsheet tools are exposed, they will be able to do this easily using the cli.

OK, I got it. I will just complete that for the long term, I would recommend not to have any CLI at all ;)

@AlexFernandes-MOVAI AlexFernandes-MOVAI self-requested a review May 12, 2022 14:50
@MithunKinarullathil
Copy link
Collaborator Author

This PR is just to close up a wound that is restricting the users without additional advantage. We have another good way to expose the ROS topics using the movai-cheatsheet, which has been tested thoroughly in the EE. It is better than exposing the full network to the host (which can have unintended consequences). So to summarize what this PR archives.

  • User is able to run their own ROS launches and ROS things in their host independently with movi-flow running beside it. (currently, this is not possible if movai-flow is running, which is restricting function without providing additional value)
  • For the short term we will document the steps from the cheat sheet, and how to expose topics manually to the community
  • For the long term, once the cheatsheet tools are exposed, they will be able to do this easily using the cli.

OK, I got it. I will just complete that for the long term, I would recommend not to have any CLI at all ;)

Haha. I have to disagree from a ros developer perspective. It's much easier for me to develop with a cli even if there is a good UI tool. It saves time ;)

@MithunKinarullathil MithunKinarullathil marked this pull request as draft May 12, 2022 15:16
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@AlexFernandes-MOVAI AlexFernandes-MOVAI added the bug Something isn't working label May 13, 2022
@MithunKinarullathil MithunKinarullathil mentioned this pull request May 16, 2022
5 tasks
@MithunKinarullathil MithunKinarullathil marked this pull request as ready for review May 23, 2022 17:24
@MithunKinarullathil MithunKinarullathil merged commit 2aa51b3 into dev May 23, 2022
@MithunKinarullathil MithunKinarullathil deleted the bugfix/remove-exposed-ros-master-port branch May 23, 2022 17:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants