-
Notifications
You must be signed in to change notification settings - Fork 128
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
Stop using Application.get_env #326
Comments
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. |
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. |
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:
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 Of course this might not be the only way, definitely open to suggestions! |
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.
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.
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 |
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! |
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 |
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. |
Ah okay. I'd love to help out with those rewrites if possible when the time comes! |
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. |
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.
|
The infrastructure should use namespaces, I'm more concerned about this from Nostrum
Privacy, like Security, requires Threat Modelling, and has layers of concerns |
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.
The text was updated successfully, but these errors were encountered: