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.
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.
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. |
leossok
force-pushed
the
feature/url_path
branch
4 times, most recently
from
90c5666
to
ad28c22
Compare
leossok
force-pushed
the
feature/url_path
branch
from
ad28c22
to
d1ff911
Compare
|
Sorry for late fix, please review. |
leossok
force-pushed
the
feature/url_path
branch
from
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.
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.
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.
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.
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
--addresstype bug. replaced type int() with str()--url_pathand 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/