-
-
Notifications
You must be signed in to change notification settings - Fork 383
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
Rewrite the .dockerignore file into a denylist #2971
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This LGTM, I'll let Potato have a look just in-case I missed anything silly :)
This PR is related to #2899, but it seems that PR has gone stale. I originally added Could we modify |
We could add building (not necessarily pushing) Docker images to the CI. However, that should probably be a separate PR. :^)
Yep, sure. I'll update the PR soon. |
e6a9603
to
6507a63
Compare
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. |
Your good... I'm not a reviewer for the tshock program. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, this makes much more sense. Hopefully this won't break in the future. 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!
This should help with not forgetting to add any new directories (such as TShockInstaller and TShockPluginManager, which have been missing until now).
6507a63
to
60599ef
Compare
Well, if you want, you can already take a look at #2974. :^) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, thanks Tim! This is good to go.
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 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. |
This should help with not forgetting to add any new directories (such as TShockInstaller and TShockPluginManager, which have been missing until now).