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

luci-compat: CBI skip fields that do not satisfy depends #7463

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jjm2473
Copy link
Contributor

@jjm2473 jjm2473 commented Dec 10, 2024

If depends is not satisfied, the fvalue will be nil.

@jow-

  • This PR is not from my main or master branch 💩, but a separate branch ✅
  • Each commit has a valid ✒️ Signed-off-by: <[email protected]> row (via git commit --signoff)
  • Each commit and PR title has a valid 📝 <package name>: title first line subject for packages
  • Incremented 🆙 any PKG_VERSION in the Makefile
  • Tested on: (aarch64, openwrt 22.03, chrome) ✅
  • ( Preferred ) Mention: @ the original code author for feedback
  • ( Preferred ) Screenshot or mp4 of changes:
  • Description: (describe the changes proposed in this PR)

A simple test code (model/cbi/abc.lua):

local m, s, o

m = Map("floatip", translate("FloatingGateway"))

s=m:section(NamedSection, "main", translate("Global settings"))
s.anonymous=true

o = s:option(Flag, "enabled", translate("Enable"))
o.rmempty = false

o = s:option(ListValue, "role", translate("Node Role"))
o.rmempty = false
o.widget = "select"
o:value("main", translate("Preempt Node"))
o:value("fallback", translate("Fallback Node"))

o = s:option(Value, "set_ip", translate("Floating Gateway IP"))
o.rmempty = false
o.datatype = "or(ip4addr,cidr4)"

o = s:option(Value, "check_ip", translate("Preempt Node IP"))
o.rmempty = false
o.datatype = "ip4addr"
o:depends("role", "fallback")

return m

Without this patch, when "role" changes from "fallback" to "main", save will failed with 'missing' error (One or more required fields have no value!).

@jjm2473 jjm2473 marked this pull request as draft December 25, 2024 10:26
@jjm2473

This comment was marked as outdated.

If depends is not satisfied, the fvalue will be nil.

Signed-off-by: Liangbin Lian <[email protected]>
@jjm2473 jjm2473 force-pushed the master-cbi-skip-not-satified branch from 3ac5741 to 4860e90 Compare December 25, 2024 12:09
@jjm2473 jjm2473 marked this pull request as ready for review December 25, 2024 12:13
@jow-
Copy link
Contributor

jow- commented Dec 25, 2024

Please do not introduce new options into the legacy Lua cbi library.

@jjm2473
Copy link
Contributor Author

jjm2473 commented Dec 26, 2024

Please do not introduce new options into the legacy Lua cbi library.

It's not an option, 'nilasempty' just a flag for internal usage, frontend pass nil when list empty.

If we don't do this, how can we fix the luci bug?

luci-app-dockerman is also affected by this bug. First switch "Remote Endpoint" to true, set "Remote Host" to "10.1.1.2", set "Remote Port" to "2375", save and apply; then switch "Remote Endpoint" to false. At this time, no matter how you set parameters, it will not be saved.

@jjm2473
Copy link
Contributor Author

jjm2473 commented Dec 26, 2024

The key point is that we should not check 'rmempty = false' when depends is not satisfied.

@jjm2473
Copy link
Contributor Author

jjm2473 commented Dec 26, 2024

Dockerman issue: #7508

@systemcrash
Copy link
Contributor

In this case, I think docker's o.rmempty = true for those two options could be the correct way forward. Have you tried that?

@jjm2473
Copy link
Contributor Author

jjm2473 commented Jan 2, 2025

@systemcrash

In this case, I think docker's o.rmempty = true for those two options could be the correct way forward. Have you tried that?

Maybe that can save. But when "Remote Endpoint" enabled, user must set "Remote Host" and "Remote Port", so o.rmempty must be false.

@systemcrash
Copy link
Contributor

so o.rmempty must be false.

why?

@jjm2473
Copy link
Contributor Author

jjm2473 commented Jan 4, 2025

why?

When "Remote Endpoint" mode, "Remote Host" and "Remote Port" are all required.

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.

3 participants