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

pgadmin4: init at 6.3 #154764

Merged
merged 2 commits into from
Feb 26, 2022
Merged

pgadmin4: init at 6.3 #154764

merged 2 commits into from
Feb 26, 2022

Conversation

gador
Copy link
Contributor

@gador gador commented Jan 12, 2022

Motivation for this change

pgadmin3 was last updated in 2017 and currently doesn't build due to an openssl insecurity.
pgadmin4 is the replacement to it, but isn't yet available in nixpkgs.

Building on the work of @mkg20001 at #128986 I finished the work on the pgadmin4 derivation.

This PR also includes some updates to recent python packages (either due to requirements by pgadmin4 or due to build errors).

Due to the way the pgadmin4 build process works, the derivation primarily builds a python wheel and includes a bundle from yarn build files.

One major thing that needs to be discussed is the way pgadmin4 in itself works: It opens a webserver on port 5050 on localhost and needs to save state in /var/lib/pgadmin4 and saves logs to /var/log/pgadmin4. One needs to either run as root, or create the directories with user permissions. The data directory can be overwritten (either at build time, or later by placing a config file in /etc/pgadmin/config_system.py). See here.

In the long run, it probably will make sense to implement a system module with a configurable port, listen address and state directory.

Right now, pgAdmin4 builds and runs (as long as the directories above have been created).

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 22.05 Release Notes (or backporting 21.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

Copy link
Member

@SuperSandro2000 SuperSandro2000 left a comment

Choose a reason for hiding this comment

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

Please split your PRs into smaller ones if you want them to be merged in time.

Please add a pythonImportsCheck to all packages.

Also can we identify the commit that causes 500+ rebuilds and push that to staging? I guess it is eventlet.

@mkg20001 do you want to maintain the packages in this PR? Also your commits are not verified.

pkgs/development/python-modules/dnspython/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/dnspython/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/eventlet/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/flask-compress/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/flask-gravatar/default.nix Outdated Show resolved Hide resolved
pkgs/tools/admin/pgadmin/default.nix Outdated Show resolved Hide resolved
pkgs/tools/admin/pgadmin/default.nix Outdated Show resolved Hide resolved
Comment on lines 140 to 166
# pgadmin offers a wide range of tests in web/regression
# unfortunately most of them require a PostgresServer in the
# test environment. Also, the data and log directories need
# to exist in the test environment.
Copy link
Member

Choose a reason for hiding this comment

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

This can be done in a VM test. We should probably do that to make sure dependency updates do not cause regressions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How would you go about it?

Copy link
Member

Choose a reason for hiding this comment

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

I didn't write a test yet but I would go into https://github.com/NixOS/nixpkgs/tree/master/nixos/tests and look at other ones and try to find something similar to my use case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @SuperSandro2000
I added the test suite which runs all python related tests. I'd love to have your feedback on them 👍

pkgs/top-level/all-packages.nix Outdated Show resolved Hide resolved
pkgs/top-level/all-packages.nix Outdated Show resolved Hide resolved
@gador gador force-pushed the pgadmin4 branch 2 times, most recently from fb5f890 to 4955bfb Compare January 14, 2022 21:45
@ofborg ofborg bot requested review from wmertens and domenkozar January 14, 2022 21:58
@gador
Copy link
Contributor Author

gador commented Jan 14, 2022

Hi @SuperSandro2000 thanks for your quick and very thorough review!
I revised everything according to your suggestions.

Do you want me to split this PR into the separate packages? Or can we leave it as one?

I reverted the chages to eventlet, but it still evaluates to 547 updated packages. Also some of those changes break packages with very narrow requirements (e.g. mcstatus has dnspython==2.1.0 as requirement). How do I find the commit that causes the mass-rebuild?

Should I split the updated packages into isolated PR's and fix any downstream breakages? How is this usually managed?

Thanks for your time!

gador added a commit to gador/nixpkgs that referenced this pull request Jan 16, 2022
@mkg20001
Copy link
Member

pgadmin3 commit message needs adjustement, should be pgadmin3: move

pgadmin4 version 6.5 came out, we could update it in this pr or leave that for later

@gador
Copy link
Contributor Author

gador commented Feb 26, 2022

pgadmin3 commit message needs adjustement, should be pgadmin3: move

fixed.

pgadmin4 version 6.5 came out, we could update it in this pr or leave that for later

I'd leave it for later. Let's first get a working pgadmin into nixpkgs and update it to 6.5 in a later PR

@mkg20001
Copy link
Member

I think maybe squash the pgadmin4 commits, it's common to just have one init commit on package creation. Then I'll merge it, seems good as is.

Signed-off-by: Florian Brandes <florian.brandes@posteo.de>
moved pgadmin3 to pgadmin4 and renamed to 3.nix
added an alias for pgadmin->pgadmin4

Signed-off-by: florian on nixos (Florian Brandes) <florian.brandes@posteo.de>
@gador
Copy link
Contributor Author

gador commented Feb 26, 2022

I think maybe squash the pgadmin4 commits, it's common to just have one init commit on package creation. Then I'll merge it, seems good as is.

done. I also (re-)added the alias. It must have gone lost on a rebase to master..

@mkg20001 mkg20001 merged commit 3668790 into NixOS:master Feb 26, 2022
@gador
Copy link
Contributor Author

gador commented Feb 26, 2022

awesome! Thanks for your help 🥳

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/cant-install-postgres4-from-unstable/18068/1

@ajs124 ajs124 mentioned this pull request Mar 14, 2022
pgadmin = callPackage ../applications/misc/pgadmin {
pgadmin4 = callPackage ../tools/admin/pgadmin { };

pgadmin3 = callPackage ../tools/admin/pgadmin/3.nix {
Copy link
Member

Choose a reason for hiding this comment

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

I know this is already merged, but what were the reasons for keeping pgadmin3 around?

Copy link
Member

@mkg20001 mkg20001 Mar 15, 2022

Choose a reason for hiding this comment

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

in case someone needs it. I think we should get rid of it after 22.05 gets released tho.

Copy link
Member

Choose a reason for hiding this comment

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

pgadmin3 was last updated in 2017 and currently doesn't build due to an openssl insecurity.

We are basically accumulating literal trash.

Copy link
Member

Choose a reason for hiding this comment

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

in case someone needs it. I think we should get rid of it after 22.05 gets released tho.

What does pgadmin3 provide that pgadmin4 doesn't? This is NixOS unstable, remove insecure software and add a changelog entry if necessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

I say prune abandonware, and leave a release note.

Copy link
Member

Choose a reason for hiding this comment

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

In case someone needs it, they can use nixpkgs 21.11, IMO.

The package will be as broken, depending on an openssl that has been EOL since 2019, on any further release as it was in 21.11.

Copy link
Member

Choose a reason for hiding this comment

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

I am for removing it with a release note, too.

@mweinelt mweinelt mentioned this pull request Mar 30, 2022
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants