@diosmosis opened this Pull Request on May 17th 2021 Member

Description:

Some shells do not define which. For those systems, cli archiving will be unavailable and the system check will reflect this, even though everything needed to use cli archiving might be there. Fixed in this PR by just calling ps and awk and checking for a success code (which we already did for ps anyway).

Review

  • [ ] Functional review done
  • [ ] Potential edge cases thought about (behavior of the code with strange input, with strange internal state or possible interactions with other Matomo subsystems)
  • [ ] Usability review done (is anything maybe unclear or think about anything that would cause people to reach out to support)
  • [ ] Security review done see checklist
  • [ ] Code review done
  • [ ] Tests were added if useful/possible
  • [ ] Reviewed for breaking changes
  • [ ] Developer changelog updated if needed
  • [ ] Documentation added if needed
  • [ ] Existing documentation updated if needed
@diosmosis commented on May 24th 2021 Member

@sgiehl @Findus23 @tsteur updated the test commands

@Starker3 commented on June 1st 2021

Not sure if this was ready to be tested yet, but using the most recent version of Process.php in this PR resulted in the Managing Processes via CLI to no longer be supported on two different testing instances where it was previously working. (Ubuntu & Debian)

@tsteur commented on June 1st 2021 Member

fyi works nicely here (on a Mac).

@tsteur commented on June 1st 2021 Member

@Starker3 maybe execute below 2 commands and let us know what it outputs and the exit code:

ps x 2>/dev/null
echo " 537 s000 Ss 0:00.05 login -pfl theuser /bin/bash -c exec -la bash /bin/bash" | awk '! /defunct/ {print $1}'
@Starker3 commented on June 1st 2021
ps x 2>/dev/null

This outputs a list of processes with PID TTY STAT TIME COMMAND
and exists with exit code 0 (Both systems)

echo " 537 s000 Ss 0:00.05 login -pfl theuser /bin/bash -c exec -la bash /bin/bash" | awk '! /defunct/ {print $1}'

This outputs 537 with exit code 0 (Both systems)

@diosmosis commented on June 1st 2021 Member

@Starker3 that's what they're supposed to return, but it's still saying it's not supported in the diagnostic?

@Starker3 commented on June 1st 2021

@diosmosis I used this version of the Process.php file when testing: https://github.com/matomo-org/matomo/blob/92256f4dcc13091c624cbe4b0f30b7b7f25dc212/core/CliMulti/Process.php
And on two different instances it returned Not supported in the diagnostic (Via web and cli)
Both instances using the normal Process.php show that Managing Processes via CLI works.

@diosmosis commented on June 1st 2021 Member

@Starker3 would you be able to try w/ the latest? That ones missing a quote and the end of the command (which should have resulted in a fatal error).

@Starker3 commented on June 1st 2021

@diosmosis I just tested using this one: https://github.com/matomo-org/matomo/blob/a7d5f34a9f91be84ed958261c32a92d983317a81/core/CliMulti/Process.php
And the results is the same: Setup Cron - Managing processes via CLI: INFORMATIONAL not supported (optional)

@diosmosis commented on June 1st 2021 Member

@Starker3 ok, I can reproduce, looking into it.

@diosmosis commented on June 1st 2021 Member

@Starker3 @tsteur there was a trim() missing. Added it + added some output in the diagnostic explaining why cli archiving isn't supported to make debugging this issue easier for support.

@Starker3 commented on June 1st 2021

@diosmosis The latest changes work for me. Tested on Debian GNU/Linux 10

@diosmosis commented on June 1st 2021 Member

@tsteur updated again. multiple reasons displayed + constants used.

@tsteur commented on June 1st 2021 Member

👍 Looks good @diosmosis worked for me

This Pull Request was closed on June 1st 2021
Powered by GitHub Issue Mirror