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

Added online debug log creator #399

Merged
merged 8 commits into from
Feb 27, 2017
Merged

Added online debug log creator #399

merged 8 commits into from
Feb 27, 2017

Conversation

DL6ER
Copy link
Member

@DL6ER DL6ER commented Feb 10, 2017

By submitting this pull request, I confirm the following (please check boxes, eg [X] - no spaces) Failure to fill the template will close your PR:

Please submit all pull requests against the development branch. Failure to do so will delay or deny your request

  • I have read and understood the contributors guide.
  • I have checked that another pull request for this purpose does not exist.
  • I have considered, and confirmed that this submission will be valuable to others.
  • I accept that this submission may not be used, and the pull request closed at the will of the maintainer.
  • I give this submission freely, and claim no ownership to its content.

How familiar are you with the codebase?: 10


Calls sudo pihole -d -a

screenshot at 2017-02-10 12-22-28

This template was created based on the work of udemy-dl.

@DL6ER DL6ER added this to the v2.5 milestone Feb 10, 2017
@dschaper
Copy link
Member

sudo pihole -d -a is going to be the trigger then?

@dschaper dschaper self-assigned this Feb 10, 2017
@DL6ER
Copy link
Member Author

DL6ER commented Feb 10, 2017

Yes. it can still be changed if you'd like to have something different

@dschaper
Copy link
Member

LGTM.

@PromoFaux
Copy link
Member

PromoFaux commented Feb 22, 2017

Can we not make it an optional upload still?

I'm thinking Modal dialog box with a "Would you like to upload the debug log once it has completed?" before the script runs.

OR alternatively a button that will just run the nc command to upload /var/log/pihole_debug.log at the end?

tl;dr I don't like (And therefore some users are not going to like) not giving users a choice.

@DL6ER
Copy link
Member Author

DL6ER commented Feb 22, 2017

I'm thinking Modal dialog box with a "Would you like to upload the debug log once it has completed?" before the script runs.

Yeah, that is possible, depending on that we can then decide whether to call pihole -d or pihole -d -a

@dschaper
Copy link
Member

dschaper commented Feb 22, 2017

pihole -d will still block though, it's going to wait for user interaction. There's probably going to be more flags needed to be passed, one for auto-no upload and one for auto-with upload. Unless there's another way around the issue.

Edit: NoTTY's don't block for read.

@PromoFaux
Copy link
Member

Maybe just lower the blocking time at the end of the script, take the interaction out of it and make it mandatory...

@dschaper
Copy link
Member

It has the timeout now, so it will stop the log capture, but then it still requires a Y/N for log upload. If the solution is that pihole -d auto-uploads, then that's a problem.

@DL6ER
Copy link
Member Author

DL6ER commented Feb 23, 2017

it still requires a Y/N for log upload

The pipe that is opened by our PHP script is read-only, hence read will fail (as if saying "No"). In fact, there is no problem:

  • pihole -d won't try to upload,
  • pihole -d -a will try to upload.

@dschaper
Copy link
Member

Yeah, I see now that it's no-TTY, so read fails, the script sees that as a non=y answer and doesn't attemtp to upload.


// Will be called when script has finished
source.addEventListener("error", function(e) {
source.close();
Copy link
Contributor

Choose a reason for hiding this comment

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

Should re-enable the button and checkbox when finished.

Copy link
Member

Choose a reason for hiding this comment

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

I'd like to keep that as it is. Force the users to at least reload the page before sending another debug.

Signed-off-by: Dan Schaper <dan.schaper@pi-hole.net>
@dschaper
Copy link
Member

dschaper commented Feb 27, 2017

Approved

Approved with PullApprove

@dschaper dschaper merged commit 110ab45 into devel Feb 27, 2017
@dschaper dschaper deleted the new/OnlineDebug branch February 27, 2017 20:30
@pralor-bot
Copy link

This pull request has been mentioned on Pi-hole Userspace. There might be relevant details there:

https://discourse.pi-hole.net/t/pi-hole-2-13-core-2-5-web/2004/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants