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

Fix/reth ports #227

Closed
wants to merge 18 commits into from
Closed

Fix/reth ports #227

wants to merge 18 commits into from

Conversation

paolofacchinetti
Copy link
Contributor

Closes #218

  • Set the default wsPort value to 8546 to reflect the default in Reth shown below:
      --ws.port <WS_PORT>
          Ws server port to listen on

          [default: 8546]
  • Unify authPort and wsAuthPort by removing the latter, since Reth has only one single authrpc.port flag

Savid and others added 6 commits September 20, 2023 13:48
* feat(charts/xatu-mimicry): add probe

* feat(charts/xatu-mimicry): add default captureDelay
* feat(chart/ethereum-node): add holesky network

* feat(chart/ethereum-node): bump version

* feat(chart/ethereum-node): bump sub charts

* feat(chart/ethereum-node): bump sub charts
@barnabasbusa barnabasbusa mentioned this pull request Sep 27, 2023
@barnabasbusa
Copy link
Collaborator

Could you please run make init, make docs and make lint. It will fail, as you forgot to bump the chart version. Every modification needs a chart version bump.
Also could you please merge in #219 as thats a very minor change, and probably not worth having two releases for such a small example change.

@paolofacchinetti
Copy link
Contributor Author

paolofacchinetti commented Sep 27, 2023

@barnabasbusa should be ok now, thanks for the feedback

as for #219, I just committed the changes myself, since the README.md.gotmpl also needed changes that weren't included in that PR

Copy link
Collaborator

@barnabasbusa barnabasbusa left a comment

Choose a reason for hiding this comment

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

LGTM

@paolofacchinetti
Copy link
Contributor Author

I see the ci lint failing, does the chart bump need to be the last pushed commit? or is that a known issue and when you squash and merge to master it will take care of itself?

@barnabasbusa
Copy link
Collaborator

Could you rebase master, and bump to 0.0.8 ?
Current master is 0.0.7 already.
https://github.com/ethpandaops/ethereum-helm-charts/blob/master/charts/reth/Chart.yaml#L10

@barnabasbusa
Copy link
Collaborator

barnabasbusa commented Sep 27, 2023

You gotta bump ethereum-nodes too, as you modified that too, then rerun the make docs. But all of this should be able to be caught if you run make lint locally btw.

@paolofacchinetti
Copy link
Contributor Author

yeah, i'm sorry for the mess but i'm having issues running make lint locally, which is weird because make docs works fine. Might be some WSL2 + Docker desktop shenanigans...

I'm getting a Error: failed linting charts: failed identifying charts to process: must be in a git repository

@barnabasbusa
Copy link
Collaborator

Looks like this PR now includes a lot more changes than a few commits before. I'm not really sure the rebase worked out the way we wanted it.

@paolofacchinetti
Copy link
Contributor Author

paolofacchinetti commented Sep 27, 2023

yeah - I wrongfully rebased master on top of this branch... I'll scrap this and make a new PR from scratch since the changes are trivial. I'll also try running make lint from a pure linux OS and not WSL so I don't end up wasting gha workflow runs

Thanks for your patience :')

@paolofacchinetti
Copy link
Contributor Author

closing this in favour of #235

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

duplicate ports on reth
4 participants