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

Add basic phpmyadmin implementation #100445

Closed
wants to merge 3 commits into from
Closed

Conversation

@mohe2015
Copy link
Contributor

@mohe2015 mohe2015 commented Oct 13, 2020

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

@aanderse aanderse commented Oct 13, 2020

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 mohe2015 commented Oct 14, 2020

@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 Oct 16, 2020
@aanderse
Copy link
Member

@aanderse aanderse commented Oct 20, 2020

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

Copy link
Member

@aanderse aanderse left a comment

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

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
Copy link
Contributor Author

@mohe2015 mohe2015 left a comment

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 Outdated Show resolved Hide resolved
@mohe2015 mohe2015 force-pushed the phpmyadmin branch 3 times, most recently from 8f43ac8 to 04d0388 Nov 1, 2020
@mohe2015 mohe2015 requested a review from aanderse Nov 1, 2020
@mohe2015
Copy link
Contributor Author

@mohe2015 mohe2015 commented Nov 1, 2020

@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

@mohe2015 mohe2015 commented Nov 27, 2020

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

@aanderse
Copy link
Member

@aanderse aanderse commented Dec 3, 2020

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

@mohe2015
Copy link
Contributor Author

@mohe2015 mohe2015 commented Feb 2, 2021

@aanderse new year, new luck ;-)

@mohe2015
Copy link
Contributor Author

@mohe2015 mohe2015 commented Apr 19, 2021

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
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants