Feat: Allow overriding channel config via .env file

Asartea commented 3 months ago

Because

582

This PR

Allows channel ID's to be overridden via .env
Modifies .env.sample to include a note on this functionality

Issue

Closes #582

Additional Information

Pull Request Requirements

[x] I have thoroughly read and understand The Odin Project Contributing Guide
[x] The title of this PR follows the location of change: brief description of change format, e.g. Callbacks command: Update verbiage
[x] The Because section summarizes the reason for this PR
[x] The This PR section has a bullet point list describing the changes in this PR
[x] If this PR addresses an open issue, it is linked in the Issue section
[x] If applicable, I have ensured all tests related to any command files included in this PR pass, and/or all snapshots are up to date

Mclilzee commented 3 months ago

At this point, I don't see a reason for us to keep our own channels hard coded at all. A maintainer can migrate the hard coded channels into the .env on the server. Since a user who is forking and cloning the bot will have no use of the hard coded values anyway, and they won't work even if they used them. I would just include all the channels into .env.

This will also reduce the logic that is in the config file after this PR.

Asartea commented 3 months ago

I have a few thoughts on this, but am currently on holiday; will see if I can get them together tomorrow

Asartea commented 3 months ago

So a few thoughts:

How would this work with tests? The text-based commands make extensive use of snapshots to check validity, and those snapshots include the embedded channel ID's. Without some way to access those, snapshot tests aren't possible in the same way
How would this work with adding new channel ID's? Right now if I need to add a new one, I can simply add an extra field to the config. If we divorce the production ID's from config, I'd need to push a PR, then ask the merging maintainer to also add the new ID to the prod env variables, but that introduces both delay (during which the command is broken), and opportunities for forgetting/mistyping.

Mclilzee commented 3 months ago

So a few thoughts:

1. How would this work with tests? The text-based commands make extensive use of snapshots to check validity, and those snapshots include the embedded channel ID's. Without some way to access those, snapshot tests aren't possible in the same way

2. How would this work with adding new channel ID's? Right now if I need to add a new one, I can simply add an extra field to the config. If we divorce the production ID's from config, I'd need to push a PR, then ask the merging maintainer to also add the new ID to the prod env variables, but that introduces both delay (during which the command is broken), and opportunities for forgetting/mistyping.

Tests shouldn't be effected by this, there should be mocked to replace the IDs with arbitrary ones. If some test were effected, then that is bad design in the test themselves, and we can refactor those. But I understand if you don't feel like working on those tests in this PR itself we can see to move forward with this, and I can refactor that later to include only IDs in .env

I don't see a problem with maintainer have to do this themselves in the .env file. There are some channels that you do not have access to, for example, and you wouldn't be able to retrieve their IDs without maintainer help. Another thing is a maintainer are able to create new channels which they will include in the .env themselves afterward it wouldn't be expected of you to come and do a PR every time a new channel is created.

Since the maintainer will merge the PR, I don't see it being a problem when they do the change afterward. I would personally still prefer a file to contain a hard coded value or an .env, having both doesn't seem necessary and add complexity / logic to configuration files.

But that's just me, maybe it's ok by the maintainers, then we can move forward without having to worry about the things you mentioned.

MaoShizhong commented 3 months ago

@TheOdinProject/odin-bot thoughts about the above suggestion from Mclilzee? Makes sense in theory to me but since it would change our workflow, I wouldn't want such a change (removing all hardcoded IDs and moving all to env vars) approved without us all being aware of the change, since I'm not sure if all of us have access to the bot deployment itself (I don't).

TheOdinProject / odin-bot-v2