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

[experiment] try using settings for peertube #119262

Closed
wants to merge 9 commits into from

Conversation

mohe2015
Copy link
Contributor

@mohe2015 mohe2015 commented Apr 12, 2021

Motivation for this change
Things done

Alternative idea to #119110

@Izorkin This probably won't make it - I just want to get some feedback regarding some problems with this approach and if that can be somehow improved. It would get a mess in the other PR.

@aanderse Pinging you is evil :D but it was your idea and I need some help. (Others are also welcome to help of course)

The important file is the module.

  1. The problem is that some additional options for e.g. redis are needed that are not in the official peertube configuration (createLocally and passwordFile in this case). Should I use a) the current approach of splitting these up into two places, b) putting the options into settings where they are currently ignored by peertube, c) put all options that are usually needed outside of settings like before (inconsistent) or do you have another idea?
  2. any idea how I can incoporate the settings.storage options without a lot of option declarations?
  3. Is the way the passwords are read in now fine (the future encrypted nix secrets provisioning may unify this whole approach AFAICT)?
  4. Anything else that comes to your mind in regard to the settings implementation?
  • 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

@mohe2015 fair. I will try to take a look in the next day or so 👍

@stevenroose
Copy link
Contributor

This seems nice. It's going to be a bit more work to maintain and it will also be tied more closely to the peertube version being used.

@mohe2015
Copy link
Contributor Author

mohe2015 commented Jul 4, 2021

Closing in favor of #119110

@mohe2015 mohe2015 closed this Jul 4, 2021
@mohe2015 mohe2015 deleted the my-add-peertube-service branch November 28, 2021 20:27
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

4 participants