Skip to content
This repository has been archived by the owner on Sep 7, 2024. It is now read-only.

Edge config gui #8

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Conversation

Danielv123
Copy link
Member

Edge transports adoption is ~0, largely due to lacking discoverability and cumbersome configuration (I presume). This PR utilizes the inputComponent plugin functionality that was added alongside modpack support 4 months ago to create a configuration popup aimed to provide as much guidance through the process and catching configuration issues as early as possible.

@Danielv123 Danielv123 added the enhancement New feature or request label May 18, 2024
@Danielv123 Danielv123 marked this pull request as ready for review May 20, 2024 13:18
@Danielv123
Copy link
Member Author

This is now working with the latest master branch of clusterio. I consider this PR fully featured at this point, although I think we should consider making changes to the edge transports config system in the future.

Currently edges are configured as part of the instance configuration, but this poses an obvious problem - which instance does an edge belong to? Since they are bidirectional they need to be configured on both the target and destination instance. You end up with 2 sources of truth which can conflict.

I propose pulling edge configurations out of the instance configurations. Instead, Id have edges be configured in a separate datastore with partner 1 and 2 (order not significant) being defined in the same json object and the changes being propagated to all partner instances. This might also help simplify and alleviate the issues that exist today with updating and synchronizing edge configuration and status like #7

Copy link
Member

@Cooldude2606 Cooldude2606 left a comment

Choose a reason for hiding this comment

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

Everything seams to work, just a few nits.

web/util/direction_to_text.jsx Outdated Show resolved Hide resolved
web/components/InputEdgeConfig.jsx Show resolved Hide resolved
Comment on lines +146 to +149
value={edge.origin?.[0]}
formatter={(value) => `x ${value}`}
parser={value => value.replace("x ", "")}
onChange={(value) => onChange({ ...edge, origin: [value, edge.origin?.[1]] })}
Copy link
Member

Choose a reason for hiding this comment

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

Will an edge ever not have an origin? Within onChange this would result in [value, undefined] which seans like it would only cause more errors.

Copy link
Member Author

Choose a reason for hiding this comment

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

It never had an origin before I defaulted it to have one. It can still not have one if defined manually using the cli for example. This pattern is required to input the array properly. If we want to avoid undefined to end up in the config like that we need a validation guard on the top level save function, not here.

web/components/InputEdgeConfig.jsx Show resolved Hide resolved
web/components/InputEdgeConfig.jsx Show resolved Hide resolved
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants