search

Pryaxis / TShock

☕️⚡️TShock provides Terraria servers with server-side characters, anti-cheat, and community management tools.
GNU General Public License v3.0
2.39k stars 374 forks source link

Rewrite the .dockerignore file into a denylist #2971

Closed timschumi closed 9 months ago

timschumi commented 10 months ago

This should help with not forgetting to add any new directories (such as TShockInstaller and TShockPluginManager, which have been missing until now).

PotatoCider commented 9 months ago

This PR is related to #2899, but it seems that PR has gone stale. I originally added .dockerignore because Docker builds were copying lots of large files in the .git/ . In hindsight, ignoring every file except project files in .dockerignore would break Docker builds with further changes (especially in the root directory).

Could we modify .gitignore to just exclude git and build directories (.git/, bin/, obj/)? You can take a look at .dockerignore in #2899. Alternatively, we could try merging #2899 instead.

timschumi commented 9 months ago

In hindsight, ignoring every file except project files in .dockerignore would break Docker builds with further changes (especially in the root directory).

We could add building (not necessarily pushing) Docker images to the CI. However, that should probably be a separate PR. :^)

Could we modify .gitignore to just exclude git and build directories (.git/, bin/, obj/)?

Yep, sure. I'll update the PR soon.

lucasgames8957 commented 9 months ago

this is... BIG

timschumi commented 9 months ago

this is... BIG

As said, the second part of the list isn't really necessary, and it doesn't have to be kept perfectly updated. It's just nice for Docker to copy and cache-track as little number of files as possible.

For now, I decided to go with the variant that keeps equivalency to the previous suggested change. If you want me to drop it, I'm happy to.

lucasgames8957 commented 9 months ago

Your good... I'm not a reviewer for the tshock program.

timschumi commented 9 months ago

CI is worth it cause it doesn't cost us anything except time, but that's a job for a future person at a future time!

Well, if you want, you can already take a look at #2974. :^)

hakusaro commented 9 months ago

I guess I don't personally like this being a denylist, because it means we can accidentally include things we don't want. I'll let someone else approve this and merge it.

timschumi commented 9 months ago

I guess I don't personally like this being a denylist, because it means we can accidentally include things we don't want. I'll let someone else approve this and merge it.

Note that "accidentally include" only means that it will get copied to the build container without being needed. It wouldn't end up in the final image (not even its history) since we copy just the dotnet publish output to a new clean base (unless dotnet publish packages up files by excessive wildcard somewhere).

If we want to do keep it as an allowlist, we should really add CI for Docker though (#2974). Otherwise it will just silently break again, just like it did for basically the entire last year.