prefer exacter matches of the vlanid when auto-selecting a related network #7901
Conversation
|
Can one of the admins verify this patch? |
2 similar comments
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
theforeman-bot
added
Waiting on contributor
Needs testing
Not yet reviewed
Legacy JS
|
Thank you for your contribution, @SimonPe! This PR has been inactive for 3 months, closing for now. |
|
I think this is still relevant. @lzap any chance you can get to reviewing this or should we find someone else? |
There was a problem hiding this comment.
I am good with the code, but I have no VLAN CR networks to test with. Can I ask for a quick test @ezr-ondrej or @shiramax ? This only works with oVirt or VMware.
- create a subnet with a VLAN ID set in Foreman
- create a oVirt/VMWare network with VLAN ID in the name
- the UI should pick the correct network automatically by finding the name
- it should ignore false matches, e.g. VLAN 4 should not match 42
app/assets/javascripts/host_edit.js
Outdated
| @@ -893,17 +893,30 @@ function selectRelatedNetwork(subnetElement) { | |||
| } | |||
|
|
|||
| var selected = null; | |||
| // prefer a match of the vlanid bounded by non digits | |||
| // this prevent vlanId=1 from matching "vlan100" | |||
| var vlanidregex = new RegExp(`(^|\\D)(${vlanId})($|\\D)`) | |||
There was a problem hiding this comment.
I think we shouldn't use template in asset pipeline, as it works only since JS ES6 and we still support older platforms. And asset pipeline doesn't transpile through babel.
| var vlanidregex = new RegExp(`(^|\\D)(${vlanId})($|\\D)`) | |
| var vlanidregex = new RegExp("(^|\\D)" + vlanId + "($|\\D)"); |
Given that vlan ids are numbers there's a big difference between vlan 23 and vlan 123 With just the substring match foreman would easily confuse the 2. This introduce a regexp matching for the VLANID and fullstring match only as a fallback.
|
ok to test |
|
I've took the liberty to force push to this branch to fix the redmine ticket relation and to fix the use of templating (I didn't change the implementation at all, only addressed the concern of compatibility issue). |
|
Green so merged, thanks all. |
|
2.3 - 21d6ea7 |
Given that vlanid's are numbers there's a big difference between vlan 23 and vlan 123
With just the substring match foreman would easily confuse the 2.
Quick drive-by patch, will look if I have the time to go trough redmine and all later, unless somebody else does that for me.
This also still falls back to the substring match which I'm not sure is needed.