Where communities thrive


  • Join over 1.5M+ people
  • Join over 100K+ communities
  • Free without limits
  • Create your own community
People
Activity
    Federico Capoano
    @nemesisdesign
    Federico Capoano
    @nemesisdesign
    openwisp/openwisp-monitoring#316 is merged! :clap: :clap:
    I spent some time planning how we can officially transition to the new package without breakages:
    https://github.com/openwisp/openwisp-monitoring/pull/302#pullrequestreview-758167928
    Kapil Bansal
    @devkapilbansal

    @devkapilbansal do you think this can be merged? https://github.com/openwisp/openwrt-openwisp-monitoring/pull/66#pullrequestreview-758166947

    Yes, it is completed

    Kapil, sometimes maintainers can be busy, look at your patches, think they're good, and merge them, breaking a software completely like it wold have happened in this case.
    For that reason it is very important that you always test your work properly, you should not send broken patches, it's very bad, moreover, if you do something like this in a new project you'll be working on it will harm your reputation or it could even prevent you from being hired.
    You must always test your work. Please also use always search and replace when renaming things.
    nickberry17
    @nickberry17
    Just letting you know ahead of time we'll be renaming device to ctl_device in the OpenWrt ModemManager protocol handler to fix an incompatibility with DSA
    Thanks to @feckert for the suggestion
    Federico Capoano
    @nemesisdesign
    great, thanks for letting us know :+1:
    we can make it compatible with both
    Federico Capoano
    @nemesisdesign
    @devkapilbansal thanks for updating the PR, did you test it yet?
    Kapil Bansal
    @devkapilbansal
    No
    Federico Capoano
    @nemesisdesign
    @devkapilbansal openwisp/openwisp-monitoring@afd6371 breaks backward compatibility, I found out only now. My testing was too quick, I didn't give time to the systems to process the latest data and it looked like it was working but it was in the truth still crunching old data.
    With that commit, data is not collected successfully unless the new agent is used, which is not what we want, we need the 2 ways to coexist while the old way is migrated to the new way, which takes time.
    I will give you more information as soon as possible.
    Federico Capoano
    @nemesisdesign
    After rolling back to the commit before the metrics are collected again, the latest master of openwisp-monitoring is broken.
    You should be able to replicate the issue by using the openwrt vm, the monitoring template and disabling the openwrt package. Charts start to show empty.
    Kapil Bansal
    @devkapilbansal
    Opened a draft PR that may fix this openwisp/openwisp-monitoring#319
    Federico Capoano
    @nemesisdesign

    Opened a draft PR that may fix this openwisp/openwisp-monitoring#319

    Thanks, I am testing it :pray:

    Federico Capoano
    @nemesisdesign
    Seems to work!
    The notifications tests have been fixed but for some reason I don't see the possibility to update that branch, could you rebase openwisp/openwisp-monitoring#319 on the latest master please @devkapilbansal?
    Federico Capoano
    @nemesisdesign
    NM I did manually
    Kapil Bansal
    @devkapilbansal
    I am late to see the message 😅
    Federico Capoano
    @nemesisdesign
    It's merged, thanks :+1:
    I'll let you know if I find more issues during testing
    Federico Capoano
    @nemesisdesign
    @devkapilbansal you flagged openwisp/openwrt-openwisp-monitoring#66 as ready for review, did you test it locally?
    Kapil Bansal
    @devkapilbansal
    Yes, I forget to update. I tested it on VM
    Federico Capoano
    @nemesisdesign
    Thanks, I will test it again and hopefully we can merge it :+1:
    Federico Capoano
    @nemesisdesign
    seems to work, but I found another issue now:
    openwisp/openwrt-openwisp-monitoring#71
    Federico Capoano
    @nemesisdesign
    I don't understand why this is happening, the output returned by netjson-monitoring --dump * looks correct
    @devkapilbansal is there an easy way I can dig deeper into this?
    Federico Capoano
    @nemesisdesign
    No clue of what is going on :joy:
    Kapil Bansal
    @devkapilbansal
    Currently only asterisk with quotes is working. Probably that's why the error is not shown there 🤔
    Federico Capoano
    @nemesisdesign

    Currently only asterisk with quotes is working. Probably that's why the error is not shown there 🤔

    also with quoted asterisk the output looks correct

    apart from the missing stats
    Kapil Bansal
    @devkapilbansal
    Hi,
    How we can avoid quoting asterisk?
    I did it in agent by escaping before passing
    But, I am not sure how to achieve it here
    Federico Capoano
    @nemesisdesign
    Hi @devkapilbansal! Now that I think about it, Probably * gets expanded before getting into our code right?
    Kapil Bansal
    @devkapilbansal
    Yes, in terminal itself
    Federico Capoano
    @nemesisdesign
    Maybe we can just document a bit better what this argument does (extract traffic statistics from interfaces), put it in the README and advise to use "*" (quoted) to extract traffic statistics from all interfaces
    Kapil Bansal
    @devkapilbansal
    Federico Capoano
    @nemesisdesign
    @devkapilbansal is this something that appeared on its own or what?
    https://github.com/openwisp/openwrt-openwisp-monitoring/pull/66#issuecomment-932517804

    Updated openwisp/openwrt-openwisp-monitoring#66

    I merged this, let's proceed with the other tasks

    Kapil Bansal
    @devkapilbansal

    @devkapilbansal is this something that appeared on its own or what?
    https://github.com/openwisp/openwrt-openwisp-monitoring/pull/66#issuecomment-932517804

    This happens because last commit fails to pass shellcheck.

    It is treating "*" as a variable and the same is commented here
    Kapil Bansal
    @devkapilbansal
    I opened openwisp/openwrt-openwisp-monitoring#73 that will fix this
    Federico Capoano
    @nemesisdesign
    oh, sorry for that, I didn't realize, thanks for adding the file to the check, ensure all bash files are checked please :pray:
    Kapil Bansal
    @devkapilbansal
    One more thing that I want to add,
    Shouldn't we use names like monitoring_agent or monitoring_init instead of monitoring.agent and monitoring.init
    I saw that bash files either have .sh extension or no extension
    Federico Capoano
    @nemesisdesign
    @devkapilbansal other packages in openwrt follow that pattern
    Kapil Bansal
    @devkapilbansal
    Ohh, I was unaware of this. Thanks for telling