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

Rewrite the .dockerignore file into a denylist #2971

Merged
merged 1 commit into from
Oct 21, 2023

Conversation

timschumi
Copy link
Contributor

@timschumi timschumi commented Sep 22, 2023

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

drunderscore
drunderscore previously approved these changes Sep 24, 2023
Copy link
Contributor

@drunderscore drunderscore left a 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 :)

@PotatoCider
Copy link
Contributor

PotatoCider commented Sep 29, 2023

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
Copy link
Contributor Author

timschumi commented Sep 29, 2023

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.

@timschumi timschumi changed the title Add TShockInstaller and TShockPluginManager to Docker Rewrite the .dockerignore file into a denylist Sep 29, 2023
@ghost
Copy link

ghost commented Sep 29, 2023

this is... BIG

@timschumi
Copy link
Contributor Author

timschumi commented Sep 29, 2023

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.

@ghost
Copy link

ghost commented Sep 29, 2023

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

Copy link
Contributor

@PotatoCider PotatoCider left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Copy link
Contributor

@drunderscore drunderscore left a 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!

docs/changelog.md Outdated Show resolved Hide resolved
This should help with not forgetting to add any new directories (such as
TShockInstaller and TShockPluginManager, which have been missing until
now).
@timschumi
Copy link
Contributor Author

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. :^)

Copy link
Contributor

@drunderscore drunderscore left a 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.

@hakusaro
Copy link
Member

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
Copy link
Contributor Author

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.

@hakusaro hakusaro merged commit 5d585bb into Pryaxis:general-devel Oct 21, 2023
6 checks passed
@timschumi timschumi deleted the new-docker-dirs branch October 21, 2023 21:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants