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

fix: ePMP gps state #8575

Merged
merged 2 commits into from Apr 17, 2018

Conversation

Projects
None yet
4 participants
@pheinrichs
Contributor

pheinrichs commented Apr 17, 2018

fixes: #8567

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

@pheinrichs

This comment has been minimized.

Contributor

pheinrichs commented Apr 17, 2018

Any idea why this would have stopped working? The above does resolve, just curious is all.

@laf

This comment has been minimized.

Member

laf commented Apr 17, 2018

The {{ $index }} at the end isn't needed (never has and I'm not sure if it would have done anything or caused issues).

Specifying the index of 0 shouldn't be needed and {{ $index }} should work fine there - are you sure you needed to change that?

@murrant

This comment has been minimized.

Member

murrant commented Apr 17, 2018

@laf {{ $index }} isn't needed here, I allowed it in the new processors stuff. I'll try to make it all consistent sometime.

@pheinrichs

This comment has been minimized.

Contributor

pheinrichs commented Apr 17, 2018

Gotcha, so it was probably running 21.1.1.7.{{$index}}.{{$index}}'. So it should be okay like this?

@laf

This comment has been minimized.

Member

laf commented Apr 17, 2018

Yes that should be fine although I think you should do index: {{ $index }} instead of index: 0

@laf

This comment has been minimized.

Member

laf commented Apr 17, 2018

@laf {{ $index }} isn't needed here, I allowed it in the new processors stuff. I'll try to make it all consistent sometime.

@murrant Would be good to standardise them although I don't think processors supports not having the {{ $index }} right now - didn't work for me earlier at least.

@laf

This comment has been minimized.

Member

laf commented Apr 17, 2018

@pheinrichs Does that change to index still work for you so we can merge?

@scrutinizer-notifier

This comment has been minimized.

scrutinizer-notifier commented Apr 17, 2018

The inspection completed: No new issues

@pheinrichs

This comment has been minimized.

Contributor

pheinrichs commented Apr 17, 2018

@laf Rediscovered and polled were a success. Good to go.

@laf

laf approved these changes Apr 17, 2018

LGTM

@laf laf merged commit 052b9d1 into librenms:master Apr 17, 2018

2 checks passed

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

@pheinrichs pheinrichs deleted the pheinrichs:fix-epmp-gps branch May 16, 2018

TheMysteriousX added a commit to TheMysteriousX/librenms that referenced this pull request May 20, 2018

fix: Fixed ePMP gps state (librenms#8575)
* Update ePMP gps state

* Update index in yaml

@lock lock bot locked as resolved and limited conversation to collaborators Jul 15, 2018

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