Stop using Application.get_env

[khionu]

The library guidelines recommend against this. This adjustment would be necessary for parts of #317, and it also prevents grabbing the the token from the runtime environment or running multiple bots from the same code base (not sure why you'd do that, but generically it's not a great limitation).

My personal use case is grabbing it from Vault, which requires going through another library or CLI at runtime.

[bdanklin]

Isnt the actual issue (and related warnings) in regards to having a config.exs in a library at all. Not because it uses get_env.

Per the guidelines you linked: "The application environment should be reserved only for configurations that are truly global"

I would say the bot token falls into that.

Removing the config.exs and providing the required config a user needs to include should suffice. Its good enough for phoenix after all.

[khionu]

Phoenix generates the code that takes advantage of the config, so this is a very different case. It would not be any more flexible for Nostrum to mimic Phoenix than the current approach. The config for Phoenix is still global, just global per module that represents a given Phoenix "instance". The token would still be universally global for Nostrum, which is not a great assumption.

[Kraigie]

This is definitely an issue, but not an easy one to tackle.

To start, the guidelines definitely recommend against what we're currently doing in Nostrum, but I also don't think we're in the wrong. Even though I don't think it matters much I'll throw in my two cents -

The guidelines state:

The application environment is global which means it becomes impossible for two dependencies to use your library in two different ways

The application environment should be reserved only for configurations that are truly global, for example, to control your application boot process and its supervision tree

For the former, I don't expect this to ever happen. Would be super weird IMO. For the latter, that is mostly what we use it for. Nostrum is launched when you start your application and needs to run some things on startup.

---

On to the meat of the subject - AFAIK most users of the library won't benefit from moving the configuration elsewhere. This is really only to enable future work, right? That said, I would like to at least keep the current functionality in place. If we find that it's too messy to do so then I totally get it and we can drop it. It may also just not be possible, but I'd have to look into it more.

It seems like we're going to have to have users start multiple processes as part of their supervision tree, right? Unless we fundamentally change how we have users consume events, they'd need to start all the "core" nostrum processes, as well as their consumer process. It may be possible to wrap their consumer up in the "core" with macro magic but that may be hiding too much away. When they create this "core" process as part of their application supervision tree, they could then pass in their options.

Of course this might not be the only way, definitely open to suggestions!

[khionu]

This is really only to enable future work, right?

It may be possible to wrap their consumer up in the "core" with macro magic [...]

I made notes about the whole complexity wrapping here. A macro is not necessary, just a default Supervisor, which can take the Token as one of the opts. This is what I would suggest we do going forward.

---

For the latter, that is mostly what we use it for. Nostrum is launched when you start your application and needs to run some things on startup.

This is exactly what needs to change for #317 that makes this issue a requirement.

[Kraigie]

This is exactly what needs to change for #317 that makes this issue a requirement.

Yep! I totally get it. I think it's a good idea we should move forward with.

I made notes about the whole complexity wrapping here. A macro is not necessary, just a default Supervisor, which can take the Token as one of the opts. This is what I would suggest we do going forward.

Oh I didn't check this out yet, taking a look in a second. The bit about the macro was if we wanted to wrap the user's consumer process they're already starting in our new "core" bundle so they're only starting one process. But I'm not sure that's a good idea!

[khionu]

[...] if we wanted to wrap the user's consumer process [...]

Imo, imposing +1 thing to their supervision tree is not something worth fretting about.

[Kraigie]

Imo, imposing +1 thing to their supervision tree is not something worth fretting about.

Definitely not, I think it could be assuaged entirely just by having a good example!

[Bentheburrito]

Hi all! If this move away from using application config to setup Nostrum is still desired, I think I can take up this issue :) I have a couple of questions though.

What is the scope of this issue exactly? Am I simply replacing calls to Application.get_env/2 with a check for that config field in the process's state? Or, since Craig would prefer to keep the current functionality for now, should we fallback to Application.get_env/2 if the config wasn't passed to the process?

[Th3-M4jor]

I think for now this move is no longer desired. That said, a rewrite of a lot of the inner workings are due imo and this change could be part of that but it wouldn't be a small undertaking.

[Bentheburrito]

Ah okay. I'd love to help out with those rewrites if possible when the time comes!

[Nezteb]

This recommendation is still valid in late 2023. 😄

[khionu]

Coming back to this, I realized that I didn't specify one of the root concerns I had at the time: multiple Discord bots in the same BEAM node. It would be a security/privacy issue to reuse the cache between two bots, so full isolation is necessary, but not possible without deploying multiple system services, unless Nostrum becomes a non-Application library. My current use case for this is running an alpha copy of the bot. I'm trying to harden a Discord bot by having the full release be part of an immutable image, but then have the alpha build hot-reload changes.

[jchristgit]

It would be a security/privacy issue to reuse the cache between two bots,

I do agree, but if privacy is paramount then I'm not sure why Discord is being used in the first place...

That said the caches should definitely be split, also from a usability view: sharing the caches would give incorrect data if you were to ask the bot how many guilds it is in or similar, unless we implement some form of row-level security. That seems a bit complicated to achieve with the cache adapters infrastructure though.

[khionu]

The infrastructure should use namespaces, I'm more concerned about this from Nostrum

but if privacy is paramount then I'm not sure why Discord is being used in the first place...

Privacy, like Security, requires Threat Modelling, and has layers of concerns