op-node-entrypoint is broken

[feld]

This was broken in #119

First, the shell script syntax is broken.

"${OP_NODE_L2_ENGINE_RPC//ws/http}"

this obviously should be

"${OP_NODE_L2_ENGINE_RPC}/ws/http"

Secondly, this won't work anyway because this endpoint returns a 404.

This change was not tested. Your v0.5.0 release has been out for a month.

Nobody noticed this for a month which is DEEPLY concerning. Is anyone actually running this chain?

[danyalprout]

Thanks for reporting @feld. Tried to repro your issue locally and it seems ok for me in bash. e.g.

bash-3.2$ TEST=http://geth:8551
bash-3.2$ echo ${TEST//http/ws}
ws://geth:8551

However the proposed change seems to not work as expected.

bash-3.2$ TEST=http://geth:8551
bash-3.2$ echo "${TEST}/http/ws"
http://geth:8551/http/ws

Are you by any change overriding OP_NODE_L2_ENGINE_RPC and does it have a second http in the URL? I wonder if we should switch the // to / to only replace the first occurrence

bash-3.2$ TEST=http://gethhttp:8551
bash-3.2$ echo ${TEST//http/ws}
ws://gethws:8551
bash-3.2$ echo ${TEST/http/ws}
ws://gethhttp:8551

[feld]

the errors I received were related to my OP_NODE_L2_ENGINE_RPC still being http://geth:8551 in my deployment instead of ws:// AND it requires bash now because the ${TEST//http/ws} format is not POSIX. Only #, ##, %, and %% are POSIX.

There is no mention of this breaking change in the release notes published with the v0.5.0 tag: https://github.com/base-org/node/releases/tag/v0.5.0

[mdehoog]

Hmmm were there any other changes? That should still not have been a breaking change:

> OP_NODE_L2_ENGINE_RPC=http://geth:8551
> echo ${OP_NODE_L2_ENGINE_RPC//ws/http}
http://geth:8551

[feld]

The real breaking change was that this requires bash for the entrypoint script now instead of sh, so those who cannot use your exact docker-compose due to their different deployment strategies were caught off guard by 4d87a51 // #123