fix: respect .gitignore in grafbase deploy
#1394
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
A user was running into issues with `grafbase deploy` yesterday, where deployments failed with a bizzare error very similar to [this one][1]. We eventually tracked this down to files in the users `.direnv` or `.devenv` folders - based on the tar-rs issue _probably_ hard links of some description. While it would be good to not choke on hardlinks, I'd argue we shouldn't have been archiving these files in the first place: they're clearly meant to stay local to the users machine. So this PR switches `walkdir` for `ignore`, which will respect `.gitignore` files etc, providing users with a sensible (and in most cases zero effort) way to exclude files from the archive we build on `grafbase deploy`. I think this is a sensible default. This _could_ have ramifications for anyone doing a `grafbase deploy` after some kind of local build step, though I don't know how likely that is really. I suggest we try it and see - if anyone complains we can revisit, maybe adding a CLI flag to change the behaviour. [1]: alexcrichton/tar-rs#313
yoav-lavi
approved these changes
Feb 29, 2024
|
I think the original reasoning was build steps, but we could try this. Nice! |
|
@obmarg although we could also explicitly ignore those dirs |
We could, but the set of files/directories we probably shouldn't be uploading is large. I don't think a blacklist approach is reasonable, without some way for users to add to that blacklist. I think having miscellaneous files/folders that shouldn't be uploaded is a far more common case than having an additional build step, so prioritising that makes sense to me. Though happy to discuss if anyone agrees. |
Built binaries: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
A user was running into issues with
grafbase deployyesterday, where deployments failed with a bizzare error very similar to this one.We eventually tracked this down to files in the users
.direnvor.devenvfolders - based on the tar-rs issue probably hard links of some description. While it would be good to not choke on hardlinks, I'd argue we shouldn't have been archiving these files in the first place: they're clearly meant to stay local to the users machine.So this PR switches
walkdirforignore, which will respect.gitignorefiles etc, providing users with a sensible (and in most cases zero effort) way to exclude files from the archive we build ongrafbase deploy. I think this is a sensible default.This could have ramifications for anyone doing a
grafbase deployafter some kind of local build step, though I don't know how likely that is really. I suggest we try it and see - if anyone complains we can revisit, maybe adding a CLI flag to change the behaviour.Fixes GB-6107