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

move PGSU out to separate package #3892

Merged
merged 3 commits into from
Apr 6, 2020

Conversation

ltalirz
Copy link
Member

@ltalirz ltalirz commented Apr 4, 2020

fixes #2837
fixes #2912
fixes #2588
fixes #3879

One of the main issues with verdi quicksetup is that it is non-trivial
to figure out how to connect as the PostgreSQL superuser in a wide
variety of operating systems and PostgreSQL setups.
This PR factors out the code which takes care of this connection into a
separate pgsu package, which can then be tested on a wide variety of setups
using CI.

@ltalirz
Copy link
Member Author

ltalirz commented Apr 4, 2020

/update-requirements

@aiida-bot-app
Copy link

aiida-bot-app bot commented Apr 4, 2020

Starting 'update-requirements' workflow for this branch.

@ltalirz
Copy link
Member Author

ltalirz commented Apr 4, 2020

@sphuber While the "update requirements" failed here (perhaps because it's a package not yet on pypi), could you please still have a look, including at pgsu?

If that looks fine, then I will put it on pypi and proceed.

@ltalirz ltalirz force-pushed the issue_2837_extract_postgres branch from cde1a78 to 361a513 Compare April 5, 2020 13:47
Copy link
Contributor

@sphuber sphuber left a comment

Choose a reason for hiding this comment

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

Thanks @ltalirz , just some minor suggestions. I will try and have a look at psgu itself now

aiida/manage/external/pgsu.py Outdated Show resolved Hide resolved
aiida/manage/external/pgsu.py Outdated Show resolved Hide resolved
aiida/manage/external/pgsu.py Outdated Show resolved Hide resolved
@ltalirz ltalirz force-pushed the issue_2837_extract_postgres branch from 361a513 to 66b0ea1 Compare April 5, 2020 17:43
One of the main issues with `verdi quicksetup` is that it is non-trivial
to figure out how to connect as the PostgreSQL superuser in a wide
variety of operating systems and PostgreSQL setups.
This PR factors out the code which takes care of this connection into a
separate package, which can then be tested on a wide variety of setups
using CI.
@ltalirz ltalirz force-pushed the issue_2837_extract_postgres branch from 66b0ea1 to ffbe6a2 Compare April 5, 2020 18:28
@ltalirz
Copy link
Member Author

ltalirz commented Apr 5, 2020

/update-requirements

@aiida-bot-app
Copy link

aiida-bot-app bot commented Apr 5, 2020

Starting 'update-requirements' workflow for this branch.

@ltalirz
Copy link
Member Author

ltalirz commented Apr 5, 2020

@csadorf It seems the update-requirements workflow already fails at the "git checkout" stage... see https://github.com/aiidateam/aiida-core/actions/runs/71254296

@ltalirz ltalirz mentioned this pull request Apr 5, 2020
@codecov
Copy link

codecov bot commented Apr 5, 2020

Codecov Report

Merging #3892 into develop will increase coverage by 0.00%.
The diff coverage is 74.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##           develop    #3892    +/-   ##
=========================================
  Coverage    78.00%   78.00%            
=========================================
  Files          457      457            
  Lines        33830    33708   -122     
=========================================
- Hits         26390    26295    -95     
+ Misses        7440     7413    -27     
Flag Coverage Δ
#django 70.00% <46.00%> (-0.06%) ⬇️
#sqlalchemy 70.84% <74.00%> (-0.04%) ⬇️
Impacted Files Coverage Δ
aiida/manage/external/pgsu.py 0.00% <0.00%> (-78.38%) ⬇️
aiida/manage/external/postgres.py 81.94% <78.04%> (+6.38%) ⬆️
aiida/cmdline/commands/cmd_setup.py 94.31% <100.00%> (+0.76%) ⬆️
aiida/manage/tests/__init__.py 88.53% <100.00%> (-0.06%) ⬇️
aiida/transports/plugins/local.py 80.46% <0.00%> (+0.25%) ⬆️

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 5c0d86f...a7c4ad5. Read the comment docs.

@ltalirz
Copy link
Member Author

ltalirz commented Apr 5, 2020

@sphuber This is ready for review

@CasperWA
Copy link
Contributor

CasperWA commented Apr 6, 2020

@csadorf It seems the update-requirements workflow already fails at the "git checkout" stage... see https://github.com/aiidateam/aiida-core/actions/runs/71254296

I had the same issue for #3599.

@csadorf
Copy link
Contributor

csadorf commented Apr 6, 2020

@ltalirz @CasperWA I can only assume that this is somehow related to the fact that you were triggering it for a branch on a fork rather than on the core repository. I will try to fix that in the next few days.

Comment on lines +169 to +171
echo.echo_info('database {} already exists!'.format(dbname))
if not click.confirm('Use it (make sure it is not used by another profile)?'):
dbname = click.prompt('new name', type=str, default=dbname)
Copy link
Contributor

Choose a reason for hiding this comment

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

I realize now (and not sure if this was already the case) that the code here is actually tightly coupled to the CLI. Is this code only used from the CLI, or should it be? Maybe not for this PR, but it would be good to think about this and see if we can make this code more general or really just move it to aiida.cmdline if it really is just meant for the CLI

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, there are many pieces like this in the code, where certain functions can be called in "interactive" mode and then start prompting.

Once we have a clear path forward how to deal with those, we should go through the entire codebase once and move them.

Copy link
Contributor

@sphuber sphuber left a comment

Choose a reason for hiding this comment

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

Thanks @ltalirz . I have one general comment/question but I think that would be for another PR so I am merging this.

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