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

Invidious: init #67664

Closed
wants to merge 12 commits into from
Closed

Invidious: init #67664

wants to merge 12 commits into from

Conversation

infinisil
Copy link
Member

@infinisil infinisil commented Aug 29, 2019

Motivation for this change

Packages invidious, an alternative frontend for YouTube. With this module, running invidious is as simple as this!

{
  services.invidious.enable = true;
}

By default it runs on http://localhost:3000

There are still some things to do before this is ready (marked as TODO in the code), but it's very much usable in its current state.


settings =
let jsonType = with lib.types; nullOr (oneOf
[ bool str int (listOf jsonType) (attrsOf jsonType) ] );
Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't know this was possibile. How does a recursive type shows up in the manual?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, it doesn't, because it gives infinite recursion first :). I added a commit to fix this by overriding the description.

@infinisil
Copy link
Member Author

Well damn, the hash changed again:

hash mismatch in fixed-output derivation '/nix/store/idrw7vk3j8dvyxh2yjdnnmq6ildbyvq4-invidious':
  wanted: sha256:1z3s9qq6bzzgwjyijffy46cby2f8n03461a0ykpwsjhlyd9ls8bf
  got:    sha256:1y062ngy0wi6aa4bnz68c6jniy6c6bmz6lcvh40gz7c85rwqm23g

@jonringer Do you happen to have the previous path still? Would love if you could upload it so we can check the difference:

nix-store --export /nix/store/j02fmn53mbpgmw4ff1m430i62sspy80d-invidious > exported

@rnhmjoj
Copy link
Contributor

rnhmjoj commented Nov 2, 2019

Status? Has the nondeterminism been tamed yet?

@infinisil
Copy link
Member Author

At least for now the hash seems to still be correct.

Regarding the PR overall: I'm kind of stuck thinking about how to do a configurable database the best. Because what I really want is to run the database on one machine and the service on another. Reason is that invidious the service doesn't work well on servers because the IP there is easily blocked, so I want to run that always on the local machine. The data however should be on a single server, so it can be used by all service instances to serve the same data, such that I don't have to sync my subscriptions manually.

The current module however only supports a local database, which I did because then you don't need a database password (uses unix user authentication).

But now the thing is: When I enable the service with a local database on a machine, it shouldn't need a password. However in order for other machines running the service to access the database, they do need a password. So should I require a database password with a local database just in case other machines want to use it? Maybe I need an allowDatabaseFromOutside option. Also maybe I need a onlyDatabase option for when I only want to run the database on a machine and not the service.

Here's a mockup of what might work:

# Invidious options
{
  options = {
    # Turn this on to have a zero configuration local invidious working
    enabled = mkEnableOption "..";

    # Enable only the service
    enableService = mkEnableOption "..";

    # Enable only the database
    database.enable = mkEnableOption "..";

    # When password is not set only allow passwordless local unix user authentication
		database.password = mkOption {};
  };

  config = mkMerge [
    (mkIf cfg.enabled {
      enableService = mkDefault true;
      database.enable = mkDefault true;
    })
    (mkIf cfg.database.enable {
      assertions = [{
        # Require a password when the database won't used locally
        assertion = !cfg.enableService -> cfg.database.password != null;
      }];
    })
    (mkIf cfg.enableService {
      # ...
    })
  ];
}

@aanderse Maybe you have some opinions on this too?

@rnhmjoj
Copy link
Contributor

rnhmjoj commented Nov 2, 2019

I would go for the allowDatabaseFromOutside approach, it should be easy enough to implement and doesn't get in the way of users that simply want to run the service locally.

I still wonder if you could use something like munge: the database could continue using unix users as authentication but it would not be limited to local users. A shared secret between the two machines is needed but it would be optional.

@infinisil
Copy link
Member Author

Ah I didn't update my comment: Initially I thought about allowDatabaseFromOutside but then I thought that just password != null is a simpler version of that. Though I guess allowDatabaseFromOutside could also open the postgres port in the firewall, and it's also easier to discover when you need it. Yeah maybe the mockup above plus that option is the way to go.

munge sounds interesting, though I'd rather not tie this module to it :)

@aanderse
Copy link
Member

aanderse commented Nov 2, 2019

@infinisil is there any issue with allowing authentication from either a password file or local socket authentication? If not, I would suggest a very small wiki document explaining such an unusual problem and the solution, with a snippet that can be pasted into configuration.nix, might be better than complicating the module with all sorts of code. Maybe an explanation stating under the scenario you described simply using passwordFile is the better option, deployed over multiple nodes.

If I have missed something simple I'm sorry, it has been a while and the conversation we had around this PR is a bit foggy in my mind.

@infinisil
Copy link
Member Author

infinisil commented Jan 8, 2020

Note: This is kind of blocked on iv-org/invidious#923, I have no idea how to solve that issue. I could downgrade to version 0.19.1 which works, but that version doesn't play all videos because it doesn't have QUIC support, and 0.20.1 does have QUIC support but it doesn't run successfully, caused by the implementation of QUIC support..

@stale
Copy link

stale bot commented Jul 9, 2020

Hello, I'm a bot and I thank you in the name of the community for your contributions.

Nixpkgs is a busy repository, and unfortunately sometimes PRs get left behind for too long. Nevertheless, we'd like to help committers reach the PRs that are still important. This PR has had no activity for 180 days, and so I marked it as stale, but you can rest assured it will never be closed by a non-human.

If this is still important to you and you'd like to remove the stale label, we ask that you leave a comment. Your comment can be as simple as "still important to me". But there's a bit more you can do:

If you received an approval by an unpriviledged maintainer and you are just waiting for a merge, you can @ mention someone with merge permissions and ask them to help. You might be able to find someone relevant by using Git blame on the relevant files, or via GitHub's web interface. You can see if someone's a member of the nixpkgs-committers team, by hovering with the mouse over their username on the web interface, or by searching them directly on the list.

If your PR wasn't reviewed at all, it might help to find someone who's perhaps a user of the package or module you are changing, or alternatively, ask once more for a review by the maintainer of the package/module this is about. If you don't know any, you can use Git blame on the relevant files, or GitHub's web interface to find someone who touched the relevant files in the past.

If your PR has had reviews and nevertheless got stale, make sure you've responded to all of the reviewer's requests / questions. Usually when PR authors show responsibility and dedication, reviewers (privileged or not) show dedication as well. If you've pushed a change, it's possible the reviewer wasn't notified about your push via email, so you can always officially request them for a review, or just @ mention them and say you've addressed their comments.

Lastly, you can always ask for help at our Discourse Forum, or more specifically, at this thread or at #nixos' IRC channel.

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jul 9, 2020
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Oct 3, 2020
@stale
Copy link

stale bot commented Jun 4, 2021

I marked this as stale due to inactivity. → More info

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 4, 2021
@rnhmjoj rnhmjoj mentioned this pull request Jun 27, 2021
@sbruder
Copy link
Contributor

sbruder commented Sep 6, 2021

I updated this PR to use the latest invidious version (they no longer provide releases and encourage users to use the latest development version). It no longer segfaults on start, but still sporadically while running (though that might be fixed now).

I also removed the manual migrations, because it should now automatically do that when check_tables is enabled. That in turn makes it possible to avoid using fetchgit by also manually providing the branch, commit and version.

@infinisil are you still interested in this?

@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Sep 6, 2021
@infinisil
Copy link
Member Author

Nice! I am still interested. I'm really quite busy recently though, so I'm not sure when I have time to update/test this. It might be better if you opened a new PR @sbruder with your updated commits

@sbruder sbruder mentioned this pull request Sep 7, 2021
12 tasks
@infinisil
Copy link
Member Author

Awesome, above PR is merged!

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

6 participants