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

Make it easily change remote and/or local media count limits #2621

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

sinoru
Copy link

@sinoru sinoru commented Feb 15, 2024

This will introduce easily configurable env to change media limits by:

GLITCH_MAX_LOCAL_MEDIA_ATTACHMENTS
GLITCH_MAX_REMOTE_MEDIA_ATTACHMENTS

This PR is some part of mastodon#27833.
This will fix #1975.

@mkody
Copy link

mkody commented Feb 17, 2024

Why make another file and not use the .env?
Sure we can use the <%= ENV %> but why make hoops and not keep things consistent with the rest of the project?
For other developers and instance admins, this adds unneeded complexity which would increase with time as things might get added to either .env or the .yml, and keeping track of that is a forthcoming nightmare.
And anticipating the "why not move everything to mastodon.yml": please don't put more burden again on instance admins and the available tools that already work great, environment variables are important for many deployments too.

@sinoru
Copy link
Author

sinoru commented Feb 17, 2024

Why make another file and not use the .env? Sure we can use the <%= ENV %> but why make hoops and not keep things consistent with the rest of the project? For other developers and instance admins, this adds unneeded complexity which would increase with time as things might get added to either .env or the .yml, and keeping track of that is a forthcoming nightmare. And anticipating the "why not move everything to mastodon.yml": please don't put more burden again on instance admins and the available tools that already work great, environment variables are important for many deployments too.

@mkody I approached it for better code management. Those value can be accessed by config.x.mastodon on any rails code. Furthermore, I think it's too much to open all values to env that don't need to changed easily, which are close to constant values. I understand this might seem like that. Or maybe I can't find a single file where the constants are gathered. If not, I think it's an approach worth considering.

I think about to create a single constant file to contains all constant across the codes: image

If this is too much, or If a lot of people disagree, I can change it to accept the opinion.

@mkody
Copy link

mkody commented Feb 17, 2024

If anything, I don't think this should be done partially and being slipped in with another change... and if not done partially, you need to consider it being either backwards compatible or make it a breaking change with plenty of warnings to instance owners.

Skipping my opinion, this PR is missing a way to handle missing keys, like default values as a fallback. You need to consider that deployments may not see file changes and do merges, like Docker-based deployments, if they have to mount the file to change this (which is... terrible, ENV variables would be better here - or this should be set in the admin UI).

I would also argue that for glitch-only features, they should be in a glitch.yml file, and either the values get merged into config.x.mastodon or we have a clearly distinct config.x.glitch.

@sinoru
Copy link
Author

sinoru commented Feb 17, 2024

If anything, I don't think this should be done partially and being slipped in with another change... and if not done partially, you need to consider it being either backwards compatible or make it a breaking change with plenty of warnings to instance owners.

Actually, If I supplies env, than it doesn't introduce backwards breakable changes.

Skipping my opinion, this PR is missing a way to handle missing keys, like default values as a fallback. You need to consider that deployments may not see file changes and do merges, like Docker-based deployments, if they have to mount the file to change this (which is... terrible, ENV variables would be better here - or this should be set in the admin UI).

I agrees with that. I need to adds some env. But even that, It will be great for easily accessible constant will be great for development.

I would also argue that for glitch-only features, they should be in a glitch.yml file, and either the values get merged into config.x.mastodon or we have a clearly distinct config.x.glitch.

Right. I missed this point. I agreed.

Copy link

This pull request has merge conflicts that must be resolved before it can be 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.

Don't remove extra images or videos from other instances
2 participants