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: curio: gui listen conflict #11944

Conversation

strahe
Copy link
Contributor

@strahe strahe commented Apr 30, 2024

Related Issues

#11941

Proposed Changes

  1. Change the default value of Subsystems.GuiAddress from :4701 to 127.0.0.1:4701.
  2. Add a --gui-listen flag to curio run and keep it in sync with curio web.

Additional Info

Checklist

Before you mark the PR ready for review, please make sure that:

  • Commits have a clear commit message.
  • PR title is in the form of of <PR type>: <area>: <change being made>
    • example: fix: mempool: Introduce a cache for valid signatures
    • PR type: fix, feat, build, chore, ci, docs, perf, refactor, revert, style, test
    • area, e.g. api, chain, state, market, mempool, multisig, networking, paych, proving, sealing, wallet, deps
  • If the PR affects users (e.g., new feature, bug fix, system requirements change), update the CHANGELOG.md and add details to the UNRELEASED section.
  • New features have usage guidelines and / or documentation updates in
  • Tests exist for new functionality or change in behavior
  • CI is green

@strahe strahe marked this pull request as draft April 30, 2024 06:03
@strahe strahe marked this pull request as ready for review April 30, 2024 07:39
@jennijuju jennijuju requested review from snadrus and magik6k May 1, 2024 02:49
Copy link
Contributor

@LexLuthr LexLuthr left a comment

Choose a reason for hiding this comment

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

Can you please rebase your PR on release/curio-beta branch and not master. All active development is current being done on release/curio-beta branch.
I am not a fan of overriding config outside of DB. This will lead us back to the old world where we had issue with config/ENV pairs.
GUI should be running only on 1 node in the whole Curio cluster which also reduces the need to override.

@strahe strahe changed the base branch from master to release/curio-beta May 3, 2024 03:34
@strahe strahe marked this pull request as draft May 3, 2024 03:36
@strahe strahe closed this May 6, 2024
@strahe
Copy link
Contributor Author

strahe commented May 6, 2024

I am not a fan of overriding config outside of DB. This will lead us back to the old world where we had issue with config/ENV pairs.

I agree with that, but the listening address is not written into the database, it is specified by flag. There is a lot of flexibility when starting the service, which I think is reasonable.

GUI should be running only on 1 node in the whole Curio cluster which also reduces the need to override.

Currently, it is possible to run multiple instances, whether it's curio web or curio run, as long as the gui is added to the layers parameter. However, the listening address --listen could be ambiguous. I will submit a new PR to resolve this issue.

@strahe strahe mentioned this pull request May 6, 2024
8 tasks
@LexLuthr
Copy link
Contributor

LexLuthr commented May 7, 2024

@strahe Quick update. We are basing everything off of Master branch now. So, please base your new PR on master. I will backport it to the release branch once merged.
Thank you for your contributions. It is great to see community interested in Curio development. We are looking forward to the new PR.

@strahe
Copy link
Contributor Author

strahe commented May 7, 2024

@LexLuthr Could you review this first? #11965

If there are no suggestions or questions, I will rebase to master again.

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.

None yet

2 participants