pi-top / pi-top-Python-SDK

pi-top's Python SDK (pitop package)
Apache License 2.0
27 stars 4 forks source link

Add switch_user common helpers #558

Closed angusjfw closed 1 year ago

angusjfw commented 1 year ago
Status Ticket/Issue
Ready/Hold Ticket

Main changes

Screenshots (feature, test output, profiling, dev tools etc)

[insert screenshots here]

Other notes (e.g. implementation quirks, edge cases, questions / issues)

Manual testing tips

-

Tag anyone who definitely needs to review or help

-

codecov[bot] commented 1 year ago

Codecov Report

Base: 57.76% // Head: 57.29% // Decreases project coverage by -0.46% :warning:

Coverage data is based on head (4e5e30d) compared to base (838874e). Patch coverage: 0.00% of modified lines in pull request are covered.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #558 +/- ## ========================================== - Coverage 57.76% 57.29% -0.47% ========================================== Files 147 148 +1 Lines 6958 7014 +56 ========================================== Hits 4019 4019 - Misses 2939 2995 +56 ``` | Flag | Coverage Δ | | |---|---|---| | unittests | `57.29% <0.00%> (-0.47%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pi-top#carryforward-flags-in-the-pull-request-comment) to find out more. | [Impacted Files](https://codecov.io/gh/pi-top/pi-top-Python-SDK/pull/558?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pi-top) | Coverage Δ | | |---|---|---| | [pitop/common/switch\_user.py](https://codecov.io/gh/pi-top/pi-top-Python-SDK/pull/558/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pi-top#diff-cGl0b3AvY29tbW9uL3N3aXRjaF91c2VyLnB5) | `0.00% <0.00%> (ø)` | | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pi-top). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pi-top)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

jcapona commented 1 year ago

I think we should probably move the get_* functions into the pitop.common.current_session_info file. Also, should our pitop.command_runner.run function use the user environment from this PR?

angusjfw commented 1 year ago

I think we should probably move the get_* functions into the pitop.common.current_session_info file.

Even though these functions resemble some in there, like get_current_user, they are actually not related to a 'current session' - you have to provide a user argument and they provide fairly static information about the system users. They could have a more common/generic use than being internals of 'switch_user.py', but I'd personally suggest leaving them there until this use case emerges. They're mostly thin wrappers on the pwd module anyway. Maybe I should prefix them an underscore or two?

Also, should our pitop.command_runner.run function use the user environment from this PR

That isn't necessary unless we want to use command_runner as a different user, in which case we could add a param or separate method to command_runner and make use of these. The intention is that you could use env_for_user alongisde other use-case-specific environment changes like we have in command_runner. So if we did it might look like:

def run_command_background(command_str: str, print_output=False, user=None) -> Popen:
    if user is not None:
      return Popen(
          split(command_str),
          env=env_for_user(user, __get_env()),  # combining the switch_user env with command_runner's get_env-
          preexec_fn=lambda: switch_user(user)
...