-
Notifications
You must be signed in to change notification settings - Fork 507
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
Feature config url_path for deployment #709
Conversation
@leossok @jtbandes The linting that is failing here (https://github.com/RobotWebTools/rosbridge_suite/runs/5040354094?check_suite_focus=true) is exactly why I opened the PR to refactor in #706 |
@leossok can you please rebase this PR to fix merge conflicts? |
<<<<<<< HEAD | ||
======= |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Source control conflict markers and a bunch of other commits showing up in this pull request. I think you should try to fix the rebase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, my mistake. Now I think I resolved those conflict.
FYI rolling is still failing CI. Let us know when you get it green and someone can review the PR. If you're stuck on why it won't pass tests, please let us know and someone can try to help. |
@amacneil The CI is also failing on the ROS2 branch. It has been for more than a month. It isn't this PR. But you should fix the CI before merging any new stuff. |
Ah, I did not notice that. Yes, we should fix that before merging further PRs. |
Cause of broken CI: #725 We should ignore it for now. |
It looks like there are a number of unrelated changes in this PR (maybe a bad merge or rebase?). I think it would be better to consider those changes separately. @leossok can you take a look at the current diff and see what is related to this feature and what is not. That would help with PR review. |
90c5666
to
ad28c22
Compare
ad28c22
to
d1ff911
Compare
Sorry for late fix, please review. |
a8524da
to
ddc2fcd
Compare
url_path = self.declare_parameter("url_path", "/").value | ||
if "--url_path" in sys.argv: | ||
idx = sys.argv.index("--url_path") + 1 | ||
if idx < len(sys.argv): | ||
url_path = str(sys.argv[idx]) | ||
else: | ||
print("--url_path argument provided without a value.") | ||
sys.exit(-1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that this is copied from above. Though I think this code is sensitive for the following error:
--option1 --option2 value2
.
So it might be good to change this in another PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there an accepted and typical args parsing library that python projects use or should be using? I'd say its outside the scope of this PR to switch but it does seem like it would avoid hand-crafted args parsing like what is happening here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/docopt/docopt looks pretty nice. I wonder how hard it would be to use it while still keeping the CLI backwards compatible with the current implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about the standard argparse module?
@MatthijsBurgh is this PR 👍 from your perspective? |
Add new --url_path command line arg and corresponding ROS param to the websocket process. This argument allows for specifying an alternative path other than the root path when running the websocket process.
Public API Changes
None
Description
--address
type bug. replaced type int() with str()--url_path
and ROS paramurl_path should be a configurable parameter because when application is deployed the port (9090) need an ingress to url path.
(for e.g. 127.0.0.0:9090 -> xxx.ap-southeast-1.elb.amazonaws.com/rosbridge_websocket)
https://kubernetes.io/docs/concepts/services-networking/ingress/