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

test whether postgres user exists #25

Merged
merged 3 commits into from
Mar 9, 2021
Merged

test whether postgres user exists #25

merged 3 commits into from
Mar 9, 2021

Conversation

ltalirz
Copy link
Member

@ltalirz ltalirz commented Mar 1, 2021

fix #18

So far, pgsu only tried the "sudo route" when it detected that it was
running on Ubuntu Linux.
It was reported that this test can fail when running on Windows
Subsystem for Linux.
Furthermore, this test might be too strict and exclude other Linux
distributions with similar postgresql setup (unverified).

Here, we switch from the check of the distribution name to a check
whether the postgres system user exists.

Furthermore, we introdyce the try_sudo and sudo_user constructor
arguments, which can be used to force pgsu to try the "sudo route" for
a given system user.

@ltalirz ltalirz force-pushed the issue_18_change_test branch 2 times, most recently from 4d25835 to 805b5a4 Compare March 1, 2021 21:48
So far, `pgsu` only tried the "sudo route" when it detected that it was
running on Ubuntu Linux.
It was reported that this test can fail when running on Windows
Subsystem for Linux.
Furthermore, this test might be too strict and exclude other Linux
distributions with similar postgresql setup (unverified).

Here, we switch from the check of the distribution name to a check
whether the `postgres` system user exists.

Furthermore, we introdyce the `try_sudo` and `sudo_user` constructor
arguments, which can be used to force `pgsu` to try the "sudo route" for
a given system user.
@ltalirz
Copy link
Member Author

ltalirz commented Mar 1, 2021

@zhubonan does this fix things for you?

@codecov-io
Copy link

codecov-io commented Mar 9, 2021

Codecov Report

Merging #25 (17dbf86) into master (f453385) will increase coverage by 0.12%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #25      +/-   ##
==========================================
+ Coverage   95.70%   95.83%   +0.12%     
==========================================
  Files           2        2              
  Lines         163      168       +5     
==========================================
+ Hits          156      161       +5     
  Misses          7        7              
Impacted Files Coverage Δ
pgsu/__init__.py 95.51% <100.00%> (+0.14%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f453385...17dbf86. Read the comment docs.

@ltalirz
Copy link
Member Author

ltalirz commented Mar 9, 2021

@zhubonan @greschd Does this make sense?

From the CI point of view it works just as well, but of course I'm not testing other distros here.
Let me know if you would like me to add any other platforms to the CI.

@greschd
Copy link
Member

greschd commented Mar 9, 2021

Yeah I think the code makes sense. I was a bit confused by the naming though: Why is it called unix_user? Shouldn't it be called something like postgres_user?

I think the reason this seems weird to me is that there are lots of "unix users" on any given system (or.. well, none), but only one of them is the postgres user.

@ltalirz
Copy link
Member Author

ltalirz commented Mar 9, 2021

Yeah I think the code makes sense. I was a bit confused by the naming though: Why is it called unix_user? Shouldn't it be called something like postgres_user?

I think the reason this seems weird to me is that there are lots of "unix users" on any given system (or.. well, none), but only one of them is the postgres user.

Yeah... the potential problem with postgres_user is that we also want to avoid confusion with the PostgreSQL database (super)user (which is also called postgres in most cases, even if there is no corresponding unix user).

I had sudo_user in some commits before but then also this was potentially confusing (user that is running sudo vs user that you change to)...

I guess postgres_unix_user or postgres_system_user would be the most ambiguity-free?

@greschd
Copy link
Member

greschd commented Mar 9, 2021

Yeah... the potential problem with postgres_user is that we also want to avoid confusion with the PostgreSQL database (super)user (which is also called postgres in most cases, even if there is no corresponding unix user).

Aah, yeah that makes sense.

I guess postgres_unix_user or postres_system_user would be the most ambiguity-free?

Yeah, I think that'd make sense. The choice between postgres_unix_user and postgres_system_user comes down to whether you may want to use the same variable on (bare, non-WSL) Windows at some point, I guess.

@zhubonan
Copy link

zhubonan commented Mar 9, 2021

Makes sense for me.
I have tested and it works well for me on WSL1 Ubuntu 18.04 Windows 10.

@ltalirz ltalirz merged commit 6641d0c into master Mar 9, 2021
@ltalirz
Copy link
Member Author

ltalirz commented Mar 9, 2021

pgsu 0.2.0 out on pypi

@greschd
Copy link
Member

greschd commented Mar 24, 2021

Thanks @ltalirz! Do you think we should relax the version constraint (currently ~=0.1.0) in aiida-core to ~=0.1, such that it will install 0.2?

@ltalirz
Copy link
Member Author

ltalirz commented Mar 24, 2021

oops, indeed!

@ltalirz ltalirz deleted the issue_18_change_test branch August 31, 2021 08:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ubuntu environment on WSL is not detected
4 participants