Skip to content

Config-Mode UI for Interface Role Assignment (v2)#2688

Merged
neocturne merged 11 commits into
freifunk-gluon:masterfrom
lemoer:pr_config_mode_ui_for_interface_roles_v2
Apr 27, 2023
Merged

Config-Mode UI for Interface Role Assignment (v2)#2688
neocturne merged 11 commits into
freifunk-gluon:masterfrom
lemoer:pr_config_mode_ui_for_interface_roles_v2

Conversation

@lemoer

@lemoer lemoer commented Oct 30, 2022

Copy link
Copy Markdown
Member

This PR is a second attempt to bring flexible role assignment for network interfaces into the config mode UI.

Implemented Features:

  • The roles "Uplink", "Mesh" and "Client" can be assigned to interfaces.
  • Roles can also be assigned to additional interfaces (beyond lan/wan/single).
  • The roles "Uplink"/"Mesh" and "Client" are mutually exclusive.

Current Limitations:

  • As of now, additional interfaces (eth, vlan, ...) have to be created manually from CLI.
  • Assigning different roles to individual LAN/WAN ports is also only possible from CLI.

Once this PR is merged, follow-up PRs will remove the limitations and extend the functionality.

This is an example how the UI look with this PR:

image

Additional interfaces (beyond lan/wan/single) are handled correctly:

image

Some references:

@github-actions github-actions Bot added 3. topic: config-mode This is about the configuration mode 3. topic: package Topic: Gluon Packages labels Oct 30, 2022
@lemoer

lemoer commented Oct 30, 2022

Copy link
Copy Markdown
Member Author

(This PR is ready for review.)

@mkg20001

Copy link
Copy Markdown
Member

This is being used successfully in the grazer mesh

@AiyionPrime AiyionPrime added this to the 2022.1.2 milestone Dec 13, 2022
@lemoer

lemoer commented Dec 13, 2022

Copy link
Copy Markdown
Member Author

Before this PR, the mesh on lan checkbox added the mesh role in addition to the client role. We (in contrast) replace the client role with the mesh role.

This means that when mesh on lan was previously enabled, the config looks like this:

config interface 'iface_lan'
	option name '/lan'
	list role 'client'
	list role 'mesh'

When you open this config in the UI, it looks like this:

image

This means that the user intent is incorrectly shown. The user intent was: mesh, but client is shown.

After pressing save, the config looks like this:

config interface 'iface_lan'
	option name '/lan'
	list role 'client'

This means that the user intent has accidentially changed.

This is not good and should be avoided using an upgrade script.

@lemoer

lemoer commented Dec 13, 2022

Copy link
Copy Markdown
Member Author
  • TODO: Modify an upgrade script.
  • TODO: Edit command line admin wiki page. (after merge)
  • TODO: Edit suggestion for site.conf.

@github-actions github-actions Bot added the 3. topic: docs Topic: Documentation label Dec 15, 2022
@lemoer lemoer force-pushed the pr_config_mode_ui_for_interface_roles_v2 branch 2 times, most recently from 7079a3f to e10eb91 Compare December 15, 2022 23:49
@lemoer lemoer force-pushed the pr_config_mode_ui_for_interface_roles_v2 branch from e10eb91 to bfce896 Compare January 4, 2023 21:14
@lemoer

lemoer commented Jan 4, 2023

Copy link
Copy Markdown
Member Author

Rebased.

@AiyionPrime

AiyionPrime commented Jan 8, 2023

Copy link
Copy Markdown
Member

Are we not still using the client interface to determine on which port to deliver the config-mode?

This allows users to get rid of all client interfaces, not the way, where its still there and just overridden by another role, but gone completely, no?

That way this PR would enable users to lock themselves out of their router for good?

edit we are not. Just tested it.

@AiyionPrime

This comment was marked as duplicate.

@AiyionPrime AiyionPrime left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not a complete review, but only the results of me checking for affected PRs in #2765.

Comment thread package/gluon-web-model/files/lib/gluon/web/view/model/mlvalue.html Outdated
@lemoer lemoer force-pushed the pr_config_mode_ui_for_interface_roles_v2 branch from 21f2d03 to 72fa9d4 Compare January 30, 2023 21:23
@rotanid rotanid modified the milestones: 2022.1.2, 2023.1 Feb 4, 2023
@lemoer lemoer dismissed AiyionPrime’s stale review February 15, 2023 20:45

It`s already adressed.

@lemoer lemoer requested a review from T-X February 15, 2023 20:50
Comment thread package/gluon-web-network/luasrc/lib/gluon/config-mode/model/admin/network.lua Outdated
@lemoer lemoer force-pushed the pr_config_mode_ui_for_interface_roles_v2 branch from 72fa9d4 to c00bf47 Compare February 16, 2023 20:49
@lemoer lemoer added the 2. status: waiting-on-review Awaiting review from the assignee but also interested parties. label Feb 22, 2023
Comment thread package/gluon-web-network/luasrc/lib/gluon/config-mode/model/admin/network.lua Outdated
Comment thread package/gluon-web-network/luasrc/lib/gluon/config-mode/model/admin/network.lua Outdated
Comment thread package/gluon-web-model/javascript/gluon-web-model.js Outdated
Comment thread package/gluon-web-network/luasrc/lib/gluon/config-mode/model/admin/network.lua Outdated
Comment thread package/gluon-web-network/luasrc/lib/gluon/config-mode/model/admin/network.lua Outdated
Comment thread package/gluon-core/check_site.lua Outdated
@lemoer lemoer force-pushed the pr_config_mode_ui_for_interface_roles_v2 branch from 7ceda0a to 93f47c8 Compare April 19, 2023 19:34
@lemoer lemoer dismissed neocturne’s stale review April 19, 2023 19:36

It's addressed.

@lemoer

lemoer commented Apr 19, 2023

Copy link
Copy Markdown
Member Author

@neoraider gave his ack for everything except for the changes to the javascript code. He will keep a browser tab open to review the javascript code.

@Djfe

Djfe commented Apr 20, 2023

Copy link
Copy Markdown
Contributor

Just a suggestion:
We could make the disabled checkboxes clickable. Clicking a disabled checkbox will uncheck and disable other checkbox(es) (in the same row) which are (mutually) exclusive to the one that was clicked.

@lemoer

lemoer commented Apr 20, 2023

Copy link
Copy Markdown
Member Author

@Djfe I would like to get this PR merged as it is. It's been around for long enough. I will not delay it even further by adding additional features which need to be reviewed again.

Feel free to open a PR including your suggestions once this is megred.

@Djfe

Djfe commented Apr 20, 2023

Copy link
Copy Markdown
Contributor

I agree. I don't want to delay this PR with my suggestion.

Comment thread package/gluon-web-model/javascript/gluon-web-model.js Outdated
Comment thread package/gluon-core/luasrc/lib/gluon/check-site.lua Outdated
Comment thread package/gluon-web-model/javascript/gluon-web-model.js Outdated
@lemoer lemoer force-pushed the pr_config_mode_ui_for_interface_roles_v2 branch from da663a5 to 1cb1dcf Compare April 24, 2023 19:12
@lemoer lemoer force-pushed the pr_config_mode_ui_for_interface_roles_v2 branch from 1cb1dcf to 19f3c9d Compare April 24, 2023 19:42
@lemoer lemoer dismissed neocturne’s stale review April 24, 2023 19:43

It's addressed.

@lemoer

lemoer commented Apr 24, 2023

Copy link
Copy Markdown
Member Author

Thanks for the reviews I addressed all suggestions.

@neocturne neocturne merged commit 10e52be into freifunk-gluon:master Apr 27, 2023
@Djfe

Djfe commented Apr 27, 2023

Copy link
Copy Markdown
Contributor

awesome! Thanks for your work @lemoer ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

2. status: waiting-on-review Awaiting review from the assignee but also interested parties. 3. topic: config-mode This is about the configuration mode 3. topic: docs Topic: Documentation 3. topic: package Topic: Gluon Packages

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants