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.

Sign up for GitHub

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

Jump to bottom

luci-app-babeld: add babeld overview #4664

Merged
merged 1 commit into from
Dec 21, 2020
Merged

luci-app-babeld: add babeld overview #4664

merged 1 commit into from
Dec 21, 2020

Conversation

PolynomialDivision
Copy link
Member

Add overview of babeld status, xroutes and routes.

@PolynomialDivision
Copy link
Member Author

babeldapp

@PolynomialDivision
Copy link
Member Author

Fixed Typo

@PolynomialDivision PolynomialDivision force-pushed the add-luci-app-babeld branch 2 times, most recently from 0eb6067 to 5096c57 Compare December 16, 2020 16:42
feckert
feckert requested changes Dec 17, 2020
View reviewed changes
Copy link
Member

@feckert feckert left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution. See my comments-

This is a status page. So why not move them to the status menu entry?
I could also imagine to add a polling to update this page?
But this can also be done afterwards.

I am not familiar with babld.
So what kind of command is this and where does the data come from?
https://github.com/openwrt/luci/pull/4664/files#diff-1dde564372ff1363073d8c79269ca9f57f5bf1b7527ec17e545d834cb2b5cf36R13

@PolynomialDivision
Copy link
Member Author

@feckert Thanks for your comments. Currently, I'm adding ubus bindings to babeld.
That is why I maybe will refactor that again and will include your changes. ;)

@PolynomialDivision PolynomialDivision force-pushed the add-luci-app-babeld branch 4 times, most recently from ba06866 to 24bc3be Compare December 18, 2020 16:32
@PolynomialDivision
Copy link
Member Author 

* space vs. tabs in controller babeld.lua or change controller to menu.d json that would be better. Please have a look at https://github.com/openwrt/luci/blob/master/applications/luci-app-clamav/root/usr/share/luci/menu.d/luci-app-clamav.json

Changed.

* Missing new line for babeld_network.lua

Changed.

This is a status page. So why not move them to the status menu entry?

What do u mean with that?

So what kind of command is this and where does the data come from?

From the local socket from babeld. Actually, I made a PR addding ubus bindings:
openwrt/routing#630

Since, this is still under discussion I would appreciate, if we could merge this app without ubus support, and then if the ubus bindings are there, I can rewrite. :)

@PolynomialDivision
Copy link
Member Author

@feckert Sry, just saw that code formating is not correct. I will change that.

@PolynomialDivision PolynomialDivision force-pushed the add-luci-app-babeld branch 2 times, most recently from 8040482 to 88980ac Compare December 18, 2020 18:17
@PolynomialDivision
Copy link
Member Author

Okay. Tested everything and fixed code format. I'm finished. :)

@feckert
Copy link
Member

feckert commented Dec 19, 2020

Okay. Tested everything and fixed code format. I'm finished. :)

Looks good so far. Except for my comments.

@PolynomialDivision PolynomialDivision force-pushed the add-luci-app-babeld branch 4 times, most recently from 1444a2b to fe08620 Compare December 19, 2020 12:14
@PolynomialDivision
Copy link
Member Author

Compile again and tested! :D Thanks a lot. I have to update my other luci apps accordginly to this. ;)

@PolynomialDivision
Copy link
Member Author

statusbabeld cleaned

@PolynomialDivision
luci-app-babeld: add babeld overview
35761aa 
Add overview of babeld status, xroutes and routes.

Signed-off-by: Nick Hainke <[email protected]>
@PolynomialDivision
Copy link
Member Author

I can not klick on request re-view. ;)
So I mention you again @feckert .

Thanks for the quick responses and the nice code review. :) This will not be the last commit to the babeld app. :)

@feckert feckert merged commit a209c9f into openwrt:master Dec 21, 2020
@feckert
Copy link
Member

feckert commented Dec 21, 2020

Thanks

@feckert feckert mentioned this pull request Dec 22, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@feckert feckert feckert approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@PolynomialDivision @feckert