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

lime-report: status reporting utility, adaptation of #457, fixes #423 #556

Open
wants to merge 6 commits into
base: master
from

Conversation

@ilario
Copy link
Member

commented Aug 11, 2019

Here I modified @aparcar's lime-report from #457.
I also added it as a dependency of lime-debug.
As a future modification, we can adapt lime-report to use liblognorm rules if installed, see here.

Please tell me if it is ok to have hardcoded tests or if they should be taken via UCI from a /etc/config/lime-report file.

@ilario ilario requested review from G10h4ck, gmarcos87 and aparcar Aug 11, 2019

@nicopace

This comment has been minimized.

Copy link
Member

commented Aug 25, 2019

Looks ok. haven't tested. guess @gmarcos87 should check it

@gmarcos87

This comment has been minimized.

Copy link
Member

commented Sep 5, 2019

Hello @ilario , I tried lime-report and it's going very well, I think it's a good idea to approve this separately from lime-paste which generated conflicts in the pull-request #457.
Something I'd like to add, maybe it could be a different pull-request or in this one, is the possibility of including your own reports. Maybe by using hooks in /etc/hotplug.d/lime-report, or wherever you think is convenient.
The idea is that others packages can contribute their debug script (lime-metrics can expose the last path to interenet for example). What do you think? Do you consider it achievable?

Thanks

@gmarcos87

This comment has been minimized.

Copy link
Member

commented Sep 5, 2019

I approve as it is, it is up to you whether you add the feature in this pull-request or not.

define Build/Compile
endef

define Package/$(PKG_NAME)/install

This comment has been minimized.

Copy link
@gmarcos87

gmarcos87 Sep 5, 2019

Member

The installation scheme could follow the scheme recommended by @spiccinini in #562

This comment has been minimized.

Copy link
@ilario

ilario Sep 5, 2019

Author Member

I don't understand, it already looks good to me, which line should be changed?

This comment has been minimized.

Copy link
@spiccinini

spiccinini Sep 10, 2019

Contributor

what marcos says is that packages/lime-report/files/lime-report.sh should be something like packages/lime-report/files/usr/bin/lime-report.sh. But then it is not renamed as lime-report so I understand that each solution has pros and cons. In this package that only a bash script exists I would use a src directory instead of the files directory that in my opinion should be used for full path names (without renaming). So maybe using packages/lime-report/src/lime-report.sh in this case is better (having .sh has automatic syntax highliting and also the scripts that remove documentation use the extension to know that it is a bash script.....maybe we can move this to use the file tool instead of using the extension)

@ilario

This comment has been minimized.

Copy link
Member Author

commented Sep 5, 2019

Something I'd like to add, maybe it could be a different pull-request or in this one, is the possibility of including your own reports. Maybe by using hooks in /etc/hotplug.d/lime-report, or wherever you think is convenient.

I was thinking to move the list of the commands and files to an /etc/config/lime-report file, would it address the issue you mention @gmarcos87 ? Did I get it correctly?
Additionally, the list of files and commands could be accepted as arguments from the command line?

@gmarcos87

This comment has been minimized.

Copy link
Member

commented Sep 5, 2019

For now this code is quite decent, if you want we can do the merge and then iterate with some solution such as hook or /etc/config...

@ilario

This comment has been minimized.

Copy link
Member Author

commented Sep 6, 2019

Both ways are ok for me.
But let me know whether hooks directory or uci entries is the preferred improvement path.

@gmarcos87

This comment has been minimized.

Copy link
Member

commented Sep 10, 2019

I think the hook logic is more extensible than the configuration file. With the configuration file you must have the stript and edit the config. With the hook you only need the script in the right place.

@ilario

This comment has been minimized.

Copy link
Member Author

commented Sep 10, 2019

Ok, so how should it work?
Each file in that directory has to be run with sh when lime-report is called?

@gmarcos87

This comment has been minimized.

Copy link
Member

commented Sep 10, 2019

This is how lime-config solves it, we can even move that to a function in lime.utils and reuse it or make a copy and paste.

for hook in fs.dir(config.hooksDir) do

@ilario

This comment has been minimized.

Copy link
Member Author

commented Sep 12, 2019

This is how lime-config solves it, we can even move that to a function in lime.utils and reuse it or make a copy and paste.

for hook in fs.dir(config.hooksDir) do

I would not use the hotplug.d directory (neither for lime-config, we could change it or even remove it (does anyone use this feature?)).
After discussing with @G10h4ck I think that a meaningful location would be /etc/lime-report/hooks, similarly to where shared-state stores its hooks.

@G10h4ck

This comment has been minimized.

Copy link
Member

commented Sep 12, 2019

for lime-config using hotplug make sense because it is a system event, and an important one that change a bunch of configurations, and yes it is used

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.