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

Adding manage_bills.php script to scripts directory #7633

Merged
merged 9 commits into from Dec 2, 2017

Conversation

Projects
None yet
3 participants
@rucarrol
Contributor

rucarrol commented Nov 5, 2017

DO NOT DELETE THIS TEXT

Please note

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

Testers

If you would like to test this pull request then please run: ./scripts/github-apply <pr_id>, i.e ./scripts/github-apply 5926

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Nov 5, 2017

Member

Thanks for this.

I've made a few changes with a git diff if you want to take a look: https://p.libren.ms/view/397b1866

Any reason you don't check hostname rather than sysName?

Can you reverse the default of -r as it could lead to confusion if it wipes out existing bills.

Member

laf commented Nov 5, 2017

Thanks for this.

I've made a few changes with a git diff if you want to take a look: https://p.libren.ms/view/397b1866

Any reason you don't check hostname rather than sysName?

Can you reverse the default of -r as it could lead to confusion if it wipes out existing bills.

@rucarrol

This comment has been minimized.

Show comment
Hide comment
@rucarrol

rucarrol Nov 5, 2017

Contributor

I've made a few changes with a git diff if you want to take a look: https://p.libren.ms/view/397b1866

Nice, thank you!

Any reason you don't check hostname rather than sysName?

Mainly as the set of devices I'm working on have a hostname which is not the same as sysName. I have no real opinion either way, but my reasoning was mostly that sysName would be what the device things it is, and hostname would be what DNS thinks it is (Which could change). Happy to pick hostname if that's the established way of doing it.

Can you reverse the default of -r as it could lead to confusion if it wipes out existing bills.

Sure - should I change the name to flush, so it becomes a bit more obvious?

Contributor

rucarrol commented Nov 5, 2017

I've made a few changes with a git diff if you want to take a look: https://p.libren.ms/view/397b1866

Nice, thank you!

Any reason you don't check hostname rather than sysName?

Mainly as the set of devices I'm working on have a hostname which is not the same as sysName. I have no real opinion either way, but my reasoning was mostly that sysName would be what the device things it is, and hostname would be what DNS thinks it is (Which could change). Happy to pick hostname if that's the established way of doing it.

Can you reverse the default of -r as it could lead to confusion if it wipes out existing bills.

Sure - should I change the name to flush, so it becomes a bit more obvious?

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Nov 5, 2017

Member

Mainly as the set of devices I'm working on have a hostname which is not the same as sysName. I have no real opinion either way, but my reasoning was mostly that sysName would be what the device things it is, and hostname would be what DNS thinks it is (Which could change). Happy to pick hostname if that's the established way of doing it.

Hostname would be better as it's unique, sysName isn't always depending on the devices :(

You could have two flags so people can pass either hostname or sysname?

Sure - should I change the name to flush, so it becomes a bit more obvious?

Flush seems good.

Member

laf commented Nov 5, 2017

Mainly as the set of devices I'm working on have a hostname which is not the same as sysName. I have no real opinion either way, but my reasoning was mostly that sysName would be what the device things it is, and hostname would be what DNS thinks it is (Which could change). Happy to pick hostname if that's the established way of doing it.

Hostname would be better as it's unique, sysName isn't always depending on the devices :(

You could have two flags so people can pass either hostname or sysname?

Sure - should I change the name to flush, so it becomes a bit more obvious?

Flush seems good.

@rucarrol

This comment has been minimized.

Show comment
Hide comment
@rucarrol

rucarrol Nov 5, 2017

Contributor

You could have two flags so people can pass either hostname or sysname?

Will do, tomorrow.

Flush seems good.

Please see [adfa1e9]adfa1e9).

Contributor

rucarrol commented Nov 5, 2017

You could have two flags so people can pass either hostname or sysname?

Will do, tomorrow.

Flush seems good.

Please see [adfa1e9]adfa1e9).

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Nov 6, 2017

Member

Looks good to me :)

Member

laf commented Nov 6, 2017

Looks good to me :)

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Nov 16, 2017

Member

@rucarrol Any joy on the last few changes?

Member

laf commented Nov 16, 2017

@rucarrol Any joy on the last few changes?

rucarrol added some commits Nov 26, 2017

@rucarrol

This comment has been minimized.

Show comment
Hide comment
@rucarrol

rucarrol Nov 26, 2017

Contributor

Hey @laf , just committed the diff :)

Contributor

rucarrol commented Nov 26, 2017

Hey @laf , just committed the diff :)

if ($bill_name == 'all') {
$bill_name = '%';
}
if ($intf_glob == 'all') {

This comment has been minimized.

@laf

laf Nov 27, 2017

Member

Seeing as you can specify all interfaces I think we should bail the script if no -c (or -i) is set otherwise interfaces are added again

@laf

laf Nov 27, 2017

Member

Seeing as you can specify all interfaces I think we should bail the script if no -c (or -i) is set otherwise interfaces are added again

@laf

Some inline comments from me, after that I think this is good.

@laf laf added the Feature label Nov 27, 2017

@laf laf added this to the 1.35 milestone Nov 27, 2017

rucarrol added some commits Nov 27, 2017

-renaming add_ports to add_ports_to_bill
- changing -i to -b for clarity sake
- Will print help if -s and -h are not set
@rucarrol

This comment has been minimized.

Show comment
Hide comment
@rucarrol

rucarrol Nov 27, 2017

Contributor

Hey @laf

Code review was much appreciated, thank you for taking the time to do this.

I changed the flow logic around a bit for detecting -h/-s, so that it'll print warnings, then help.

Not setting -i will also cause the script to bail out.

I also renamed the create_bill function to add_ports_to_bill (So I would not clobber the add_ports functioning already existing).

Cheers + thanks against for assisting.

Contributor

rucarrol commented Nov 27, 2017

Hey @laf

Code review was much appreciated, thank you for taking the time to do this.

I changed the flow logic around a bit for detecting -h/-s, so that it'll print warnings, then help.

Not setting -i will also cause the script to bail out.

I also renamed the create_bill function to add_ports_to_bill (So I would not clobber the add_ports functioning already existing).

Cheers + thanks against for assisting.

rucarrol added some commits Nov 27, 2017

@scrutinizer-notifier

This comment has been minimized.

Show comment
Hide comment
@scrutinizer-notifier

scrutinizer-notifier Nov 27, 2017

The inspection completed: 3 new issues, 4 updated code elements

scrutinizer-notifier commented Nov 27, 2017

The inspection completed: 3 new issues, 4 updated code elements

@laf laf merged commit 731261b into librenms:master Dec 2, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details
@lock

This comment has been minimized.

Show comment
Hide comment
@lock

lock bot May 16, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed.

lock bot commented May 16, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed.

@lock lock bot locked as resolved and limited conversation to collaborators May 16, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.