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

Enable LFS #47

Merged
merged 4 commits into from
Jan 28, 2024
Merged

Enable LFS #47

merged 4 commits into from
Jan 28, 2024

Conversation

purplg
Copy link
Contributor

@purplg purplg commented Dec 11, 2023

I had to enable this when I used this template. I don't think there's a reason to not enable it by default. I used these changes in my Bevy Jam 4 submission

Thanks

Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

My concern with turning this on is that Github is very stingy with LFS bandwidth, especially for private repos. Did you run into that?

@purplg
Copy link
Contributor Author

purplg commented Dec 11, 2023

My concern with turning this on is that Github is very stingy with LFS bandwidth, especially for private repos. Did you run into that?

No, I didn't run into any issues with that. I ran all the release jobs probably around 5 or more times the last few days, so not an excessive amount. My assets are rather small too. I wasn't aware of the stingyness.

@mockersf
Copy link
Member

mockersf commented Dec 14, 2023

Without paying for more, you can have 1GB of data in git lfs, and 1GB per month of bandwidth. People cloning your repo counts against that quota.

does enabling lfs on checkout on a repo without lfs enabled have any side effect?

@purplg
Copy link
Contributor Author

purplg commented Dec 15, 2023

Alternatively, maybe an env var can be added at the top, alongside binary and add_binaries_to_github_release, which defaults to false to easily enable LFS for all the targets.

@purplg
Copy link
Contributor Author

purplg commented Dec 16, 2023

I made the change and gave it a quick test on a simple hello world repo here. There's 2 releases, one with LFS disabled where the png is a pointer file and the other with LFS enabled where the asset is binary, as expected.

@mockersf
Copy link
Member

could you add a disclaimer to really look at GitHub pricing for lfs before thinking about using it? with a link to https://docs.github.com/en/repositories/working-with-files/managing-large-files/about-storage-and-bandwidth-usage

@purplg
Copy link
Contributor Author

purplg commented Jan 27, 2024

@mockersf Thanks, definitely a good idea. I've updated the branch.

Turns out, GitHub has a beta feature to show "note", and other, blocks which is perfect for this. I've chosen "warning". Please let me know if you think a different level is better or if these blocks should be avoided for now

Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

Great choice. Agreed on warning. I might even strengthen the warning: the storage is unusually expensive at game scales.

@purplg
Copy link
Contributor Author

purplg commented Jan 28, 2024

I might even strengthen the warning: the storage is unusually expensive at game scales.

Maybe, but as I understand it, enabling this option doesn't necessarily increase quota usage directly, but rather just enable LFS within the pipelines (so the pointer files are resolved). I made a few mistakes enabling this myself, which cost me workflow minutes, and eventually money as I ran out of the allotted free workflow time. So I wanted to contribute this back in hopes it'd save others from my mistakes.

But I still think the "warning" is warranted as to not provide some false sense of security around LFS and to encourage the user to understand and decide for them self.

@mockersf mockersf merged commit bbce9df into bevyengine:main Jan 28, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants