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

refactor of daily.sh #4920

Merged
merged 2 commits into from Nov 11, 2016
Merged

refactor of daily.sh #4920

merged 2 commits into from Nov 11, 2016

Conversation

Gorian
Copy link
Contributor

@Gorian Gorian commented Nov 1, 2016

Please note

Please read this information carefully. You can run ./scripts/pre-commit.php to check your code before submitting.

Refactored code of daily.sh. Replaced -ne with "!=", "[" with "[[", enforced better quotes usage, moved daily.php calls to a dedicated function, added support for using $config['log_dir'] from config.php for setting the log directory. Defaults to the value of $config['install_dir'] . '/logs' if $config['log_dir'] is not set. Properly moved all code not in a dedicated function to main().

Refactored code of daily.sh. Replaced -ne with "!=", "[" with "[[", enforced better quotes usage, moved daily.php calls to a dedicated function, added support for using $config['log_dir'] from config.php for setting the log directory. Defaults to the value of $config['install_dir'] . '/logs' if $config['log_dir'] is not set.
@LibreNMS-CI
Copy link

Auto-Deploy finished, Test PR at http://4920.ci.librenms.org or https://4920.ci.librenms.org

Major refactor, added name to authors list :p
@LibreNMS-CI
Copy link

Auto-Deploy finished, Test PR at http://4920.ci.librenms.org or https://4920.ci.librenms.org

@scrutinizer-notifier
Copy link

The inspection completed: No new issues

@laf
Copy link
Member

laf commented Nov 3, 2016

Tested and works for me. CentOS 7.

bash --version
GNU bash, version 4.2.46(1)-release (x86_64-redhat-linux-gnu)

What have you tested against @Gorian ?

@Gorian
Copy link
Contributor Author

Gorian commented Nov 3, 2016

@laf also CentOS7, same bash version.

@jonathon-k
Copy link
Contributor

Works for me as far as I can tell. Debian Jessie
GNU bash, version 4.3.30(1)-release (x86_64-pc-linux-gnu)

@laf
Copy link
Member

laf commented Nov 8, 2016

I think we need some older distros to test this on.

Copy link
Member

@murrant murrant left a comment

Choose a reason for hiding this comment

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

I can't test older, but I can test newer.
GNU bash, version 4.4.0(1)-release (x86_64-pc-linux-gnu)

@murrant
Copy link
Member

murrant commented Nov 9, 2016

Tested on GNU bash, version 3.2.25(1)-release (x86_64-redhat-linux-gnu) too (although I didn't have a fully installed librenms there)

@laf
Copy link
Member

laf commented Nov 9, 2016

Tagged to merge in 48 hours.

@laf laf merged commit 7681580 into librenms:master Nov 11, 2016
@Gorian Gorian deleted the patch-1 branch November 11, 2016 18:53
@lock lock bot locked as resolved and limited conversation to collaborators Jan 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants