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

Jump to bottom

Add basic phpmyadmin implementation #100445

Closed
wants to merge 3 commits into from
Closed

Add basic phpmyadmin implementation #100445

wants to merge 3 commits into from

Conversation

mohe2015
Copy link
Contributor

Motivation for this change

phpmyadmin is useful to quickly do database changes or backups / recoveries.

Things done

Added phpmyadmin package.
Added phpmyadmin module.

  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@aanderse
Copy link
Member

A couple initial thoughts without having given this a thorough review:

  • I'm always shocked to see new modules using httpd instead of nginx - what was your rationale here?
  • phpmyadmin security can be questionable and updates are essential to get out on time; what is your plan and commitment level to this package?
  • configuring phpmyadmin should usually be pretty customized so can your module adequately satisfy these needs while adding an advantage over the sysadmin installing/configuring phpmyadmin on their own without a module?

@mohe2015
Copy link
Contributor Author

mohe2015 commented Oct 14, 2020 •
edited

@aanderse

I'm always shocked to see new modules using httpd instead of nginx - what was your rationale here?

wordpress module is written for httpd and I'm still using httpd. I didn't want to run two webservers. An abstraction layer seems to be really needed but I won't be able to write one. I won't rewrite this but I'm fine if this is not merged.

phpmyadmin security can be questionable and updates are essential to get out on time; what is your plan and commitment level to this package?

I'm watching the phpmyadmin repo and should be able to provide timely updates.

configuring phpmyadmin should usually be pretty customized so can your module adequately satisfy these needs while adding an advantage over the sysadmin installing/configuring phpmyadmin on their own without a module?

I added code to configure phpmyadmin. This has the advantage of NixOS, namely being declarative.

@mohe2015 mohe2015 force-pushed the phpmyadmin branch 3 times, most recently from a6bb8bf to adb59f1 Compare October 16, 2020 14:04
@aanderse
Copy link
Member

All reasonable answers, thanks. I'll try to get back to reviewing this as soon as I can.

aanderse
aanderse requested changes Oct 31, 2020
View reviewed changes
Copy link
Member

@aanderse aanderse left a comment

Choose a reason for hiding this comment

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

Sorry this took me so long to get back to. Let me know what you think.

nixos/modules/services/web-apps/phpmyadmin.nix Show resolved Hide resolved
nixos/modules/services/web-apps/phpmyadmin.nix Outdated Show resolved Hide resolved
nixos/modules/services/web-apps/phpmyadmin.nix Outdated Show resolved Hide resolved
pkgs/servers/web-apps/phpmyadmin/default.nix Outdated Show resolved Hide resolved
pkgs/servers/web-apps/phpmyadmin/default.nix Outdated Show resolved Hide resolved
pkgs/servers/web-apps/phpmyadmin/default.nix Outdated Show resolved Hide resolved
mohe2015
mohe2015 commented Oct 31, 2020
View reviewed changes
Copy link
Contributor Author

@mohe2015 mohe2015 left a comment •
edited

Choose a reason for hiding this comment

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

I really appreciate the effort you put in the review and it also teached me a few things.

For the other changes I agree with you. I'm not sure when I will find time to implement them though as next week is my first week of studying Computer Science at an University. There are not too many changes if you disregard the "multiple instances" one so I hope soon.

nixos/modules/services/web-apps/phpmyadmin.nix Show resolved Hide resolved
nixos/modules/services/web-apps/phpmyadmin.nix Outdated Show resolved Hide resolved
@mohe2015 mohe2015 force-pushed the phpmyadmin branch 3 times, most recently from 8f43ac8 to 04d0388 Compare November 1, 2020 14:38
@mohe2015
Copy link
Contributor Author

mohe2015 commented Nov 1, 2020 •
edited

@aanderse I think I addressed everything. I also added extraHttpdConfig to allow users to improve the security a lot. Let me know what you think please.

@mohe2015
Copy link
Contributor Author

@aanderse Friendly reminder if you're still interested in reviewing again or merging.

@aanderse
Copy link
Member

aanderse commented Dec 3, 2020

Apologies. I'll try to find some time to prioritize this.

@mohe2015
Copy link
Contributor Author

mohe2015 commented Feb 2, 2021

@aanderse new year, new luck ;-)

@mohe2015
Copy link
Contributor Author

I will close this for now. https://www.adminer.org/ is also probably a more "modern" solution

@mohe2015 mohe2015 closed this Apr 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aanderse aanderse Awaiting requested review from aanderse

Assignees
No one assigned
Labels
6.topic: nixos 8.has: module (update) 8.has: package (new) 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 11.by: package-maintainer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@mohe2015 @aanderse