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.

Sign up for GitHub

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

Jump to bottom

Add iOS snapshot files to Git LFS #546

Merged
merged 5 commits into from
Jul 11, 2021
Merged

Add iOS snapshot files to Git LFS #546

merged 5 commits into from
Jul 11, 2021

Conversation

takahirom
Copy link
Member

@takahirom takahirom commented Jul 10, 2021 •
edited
Loading

Issue

Overview (Required)

  • Trying to store iOS snapshot to Git LFS
  • I haven't rewritten past commits, so it won't disappear from the Git object, but future changes will no longer be saved. 👀

Links

Screenshot

Before After

@takahirom takahirom requested a review from ry-itto July 10, 2021 04:35
@github-actions github-actions bot temporarily deployed to deploygate-distribution July 10, 2021 04:40 Inactive
@takahirom takahirom force-pushed the add-git-lfs-to-snapshots branch from 3fd9825 to 7e2eef1 Compare July 10, 2021 04:42
@github-actions github-actions bot temporarily deployed to deploygate-distribution July 10, 2021 04:49 Inactive
@jmatsu
Copy link
Contributor

jmatsu commented Jul 10, 2021 •
edited
Loading

@takahirom
I also think that LFS can resolve the concern and it seems you have properly configured LFS for the snapshot files. However, this PR didn't trigger the snapshot testing, right? We should address the two points below.

  • Make sure the workflow works with Git-LFS. We need to enable LFS option of actions/checkout and add a step that pulls LFS files (and does cache, ideally).
  • How to manage them should be documented. For example, LFS files won't be downloaded by default but they're required for iOS snapshot testing.

@takahirom
Copy link
Member Author

It seems that we should add lfs: true for actions/checkout 👍

We can add the document here.
https://github.com/DroidKaigi/conference-app-2021/tree/main/ios#snapshot-testing

@github-actions github-actions bot temporarily deployed to deploygate-distribution July 10, 2021 05:49 Inactive
@github-actions github-actions bot temporarily deployed to deploygate-distribution July 10, 2021 06:24 Inactive
ry-itto
ry-itto reviewed Jul 10, 2021
View reviewed changes
Copy link
Member

@ry-itto ry-itto left a comment

Choose a reason for hiding this comment

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

Thank you for applying this with git-lfs 🙏🏼
and currently, CI job for snapshot testing is failed.

I think applying commented change may work. Please try it 🙏🏼

I have a concern about using git-lfs.
Using git-lfs can cost DroidKaigi organization money, is that okay?
( git-lfs bandwidth limit is 1GB/month for free. )

Reference:
https://docs.github.com/ja/billing/managing-billing-for-git-large-file-storage/about-billing-for-git-large-file-storage#

.github/workflows/ios-test.yml Outdated
Comment on lines 43 to 44
with:
lfs: true
Copy link
Member

Choose a reason for hiding this comment

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

It seems to call git lfs checkout is needed 🙏🏼
Please add that call step.

git lfs fetch - Downloads LFS objects referenced in current commit to .git/lfs/objects
git lfs checkout - Replaces LFS pointers in working directory with LFS objects in .git/lfs/objects
git lfs pull - Combination of git lfs fetch + git lfs checkout

Reference:
git-lfs/git-lfs#2257 (comment)

.github/workflows/ios-test.yml Outdated
Comment on lines 13 to 14
with:
lfs: true
Copy link
Member

Choose a reason for hiding this comment

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

In iOS build job, screenshot files are not needed. So it's okey to remove this 👍🏼

Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
with:
lfs: true

.github/workflows/ios-test.yml Outdated
Comment on lines 73 to 74
with:
lfs: true
Copy link
Member

Choose a reason for hiding this comment

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

ditto

In iOS build job, screenshot files are not needed. So it's okey to remove this 👍🏼

Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
with:
lfs: true

.github/workflows/ios-test.yml Outdated
Comment on lines 103 to 104
with:
lfs: true
Copy link
Member

Choose a reason for hiding this comment

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

ditto

In iOS build job, screenshot files are not needed. So it's okey to remove this 👍🏼

Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
with:
lfs: true

@takahirom
Copy link
Member Author

I have a concern about using git-lfs.
Using git-lfs can cost DroidKaigi organization money, is that okay?
( git-lfs bandwidth limit is 1GB/month for free. )

Thank you for checking 🙏
I think it is ok. 👍 But I don't want to waste it, so I would like to save it by reducing the image size. 👀

@takahirom
Copy link
Member Author

Probably we can cache by using this 👀
actions/checkout#165 (comment)

@github-actions github-actions bot temporarily deployed to deploygate-distribution July 11, 2021 06:06 Inactive
ry-itto
ry-itto approved these changes Jul 11, 2021
View reviewed changes
Copy link
Member

@ry-itto ry-itto left a comment

Choose a reason for hiding this comment

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

Looks good 👍🏼

But I don't want to waste it, so I would like to save it by reducing the image size. 👀

If above is okey with caching git-lfs steps, please merge this 🙏🏼

@takahirom
Copy link
Member Author

It may be necessary to resize the image, so I would like to ask if that happens. 🙏

@takahirom takahirom merged commit 22a93e1 into main Jul 11, 2021
@takahirom takahirom deleted the add-git-lfs-to-snapshots branch July 11, 2021 08:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ry-itto ry-itto ry-itto approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

About iOS snapshot in main branch
3 participants
@takahirom @jmatsu @ry-itto