chore: use solo node instead of local node (#5060)#5234
chore: use solo node instead of local node (#5060)#5234jasuwienas wants to merge 7 commits intomainfrom
Conversation
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | -6 |
| Duplication | -6 |
TIP This summary will be updated as you push new changes. Give us feedback
Signed-off-by: Mariusz Jasuwienas <jasuwienas@gmail.com>
eef0e7b to
121e0ed
Compare
quiet-node
left a comment
There was a problem hiding this comment.
Thanks for taking on the challenge of upgrading the versions! I’ve done it once so I know how tough it can be. Great work! Left some Qs and concerns
There was a problem hiding this comment.
So this script works around Solo's port-forwarding, but hiero-ledger/solo#3833 was fixed and seems like it still failed? I think if there's still a bug specifically with falcon deploy, we should file it on solo repo so it gets fixed upstream, rather than a workaround to fix at the consumer level like this. Or maybe we can go with this workaround now, create a ticket on Solo, then add a TODO here to track. If it's fixed upstream, it can avoid unnecessary complexity for our CI.
There was a problem hiding this comment.
I’ll update the description to better clarify what’s currently missing in the solo implementation.
What I'm actually missing in solo is flag like:
--external-address
so I can bind solo to the address: --address 0.0.0.0
Also, I believe it was your suggestion to remove HAProxy and call the underlying services directly.
There was a problem hiding this comment.
If you know any workaround for that let me know please. I'm keeping it as is for now, since some of the tests (dapp for example) require the port to be available from the container level as well, not only on the localhost.
There was a problem hiding this comment.
Thanks for updating the description. It's much clearer now. Would it make sense to file a ticket on the Solo repo for the missing --external-address flag, or any issue, and add # TODO(solo#XXXX) comments at the top of this script? That way we have a clear path to remove this workaround once it's fixed upstream.
|
|
||
| - name: Start the solo node | ||
| run: | | ||
| npx @hashgraph/solo@${{ inputs.soloVersion || '0.68.0' }} one-shot falcon deploy \ |
There was a problem hiding this comment.
It looks like soloVersion isn't defined as an input in the workflow_call section. This means the || '0.68.0' fallback will always be used and callers like manual-testing.yml can't override it. Should we add soloVersion as a proper input to all the reusable workflows?
There was a problem hiding this comment.
With this update, the networkTag and mirrorTag will no longer be used (here and in other workflows)
I think a nice way to handle this is to add a new setup-value-file step. This step can load the default falcon.yml, then dynamically loads the networkTag and mirrorTag. That way, anyone who still wants to run the test using specific tags can continue to do so.
There was a problem hiding this comment.
Also about the flags,
- Falcon alreayd have --force-port-forward set to true, right? I think we don't need to set it explicitly. In fact, should we make --force-port-forward=false as we have our own portforward setup anyway?
- We should add --deploy-explorer=false to save time
There was a problem hiding this comment.
Yes, right, we have to create the config file dynamically (and nice idea with the explorer, we DON'T need it)
There was a problem hiding this comment.
Thanks for addressing these other two. A few things I noticed about the flags:
-
The dynamically generated
falcon.ymlstill has--force-port-forward: trueunderconsensusNodein all workflows. Since we have our own port-forward script, should we remove this or set it to false? -
dapp.ymlis missing--deploy-explorer=falsethat the other workflows have. -
In
subgraph.yml, the CLI has--force-port-forward=falsebut the config file has--force-port-forward: true. These might conflict.
Signed-off-by: Mariusz Jasuwienas <jasuwienas@gmail.com>
Signed-off-by: Mariusz Jasuwienas <jasuwienas@gmail.com>
Signed-off-by: Mariusz Jasuwienas <jasuwienas@gmail.com>
Signed-off-by: Mariusz Jasuwienas <jasuwienas@gmail.com>
Signed-off-by: Mariusz Jasuwienas <jasuwienas@gmail.com>
Signed-off-by: Mariusz Jasuwienas <jasuwienas@gmail.com>
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #5234 +/- ##
=======================================
Coverage 95.93% 95.93%
=======================================
Files 146 146
Lines 25140 25182 +42
Branches 2044 2046 +2
=======================================
+ Hits 24117 24159 +42
Misses 1001 1001
Partials 22 22 see 3 files with indirect coverage changes 🚀 New features to boost your workflow:
|
quiet-node
left a comment
There was a problem hiding this comment.
Looking great left some extra item on the new updates
| run: .github/scripts/forward-ports.sh | ||
|
|
||
| - name: Start Redis | ||
| run: docker run -d --name redis -p 6379:6379 redis:7-alpine |
There was a problem hiding this comment.
Nice one! However it looks like there are still a couple of leftover local-node commands at line 129 and 132. Let's remove them
|
|
||
| - name: Start the solo node | ||
| run: | | ||
| npx @hashgraph/solo@${{ inputs.soloVersion || '0.68.0' }} one-shot falcon deploy \ |
There was a problem hiding this comment.
Thanks for addressing these other two. A few things I noticed about the flags:
-
The dynamically generated
falcon.ymlstill has--force-port-forward: trueunderconsensusNodein all workflows. Since we have our own port-forward script, should we remove this or set it to false? -
dapp.ymlis missing--deploy-explorer=falsethat the other workflows have. -
In
subgraph.yml, the CLI has--force-port-forward=falsebut the config file has--force-port-forward: true. These might conflict.
Description
Replaces hedera local node with hiero solo node.
Updates the CN and MN versions and fixes the tests to work correctly with them:
Related issue(s)
Fixes #5060
Testing Guide
Changes from original design (optional)
Additional work needed (optional)
N/A
Checklist