ubuntu / authd

Authentication daemon for external Brokers
GNU Lesser General Public License v3.0
58 stars 8 forks source link

Improve issue template #485

Closed adombeck closed 2 weeks ago

adombeck commented 3 weeks ago

Adds a comment to show all relevant system information and makes various minor improvements. See commit messages for details.

UDENG-4099

adombeck commented 3 weeks ago

reviewer: You can try the new template by creating an issue in my authd fork: https://github.com/adombeck/authd/issues/new/choose

codecov-commenter commented 3 weeks ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 84.79%. Comparing base (17530d2) to head (a64a3db). Report is 8 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #485 +/- ## ========================================== - Coverage 84.94% 84.79% -0.15% ========================================== Files 79 79 Lines 6927 6927 Branches 75 75 ========================================== - Hits 5884 5874 -10 - Misses 728 733 +5 - Partials 315 320 +5 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

adombeck commented 3 weeks ago

However over SSH without a graphical session, the script fails with the following error

Ah, good catch! I'm not sure what's the best way to handle that. Maybe check if $DISPLAY is set and if it's not, use $EDITOR to open the file?

jibel commented 3 weeks ago

However over SSH without a graphical session, the script fails with the following error

Ah, good catch! I'm not sure what's the best way to handle that. Maybe check if $DISPLAY is set and if it's not, use $EDITOR to open the file?

You could replace if command -v xdg-open > /dev/null; then xdg-open "$TMPFILE"; else $EDITOR "$TMPFILE"; fi by editor

It'll open nano by default but it shouldn't be a problem since the command has to be executed from a terminal.

adombeck commented 3 weeks ago

You could replace if command -v xdg-open > /dev/null; then xdg-open "$TMPFILE"; else $EDITOR "$TMPFILE"; fi by editor

I think that using a graphical editor when in a graphical session is better UX. We could replace $EDITOR with editor though.

adombeck commented 3 weeks ago

With the change I just pushed, it now uses xdg-open only if $DESKTOP is set, else it uses $EDITOR if set, else it uses editor.

jibel commented 3 weeks ago

With the change I just pushed, it now uses xdg-open only if $DESKTOP is set, else it uses $EDITOR if set, else it uses editor.

Thanks for this update, it works well. More hint that we need a script to collect these data :)

adombeck commented 3 weeks ago

I noticed that xdg-open && rm caused a race, because xdg-open returns immediately after spawning the process, so sometimes the file was removed before it was being opened by the editor. I pushed a fixup commit which omits the rm, so that the tmpfile is now kept in /tmp. IMO that's not too bad, but another reason to replace the command with a script soon.