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

chore: check images for fastlane and docs #488

Merged
merged 1 commit into from
Sep 6, 2019
Merged

chore: check images for fastlane and docs #488

merged 1 commit into from
Sep 6, 2019

Conversation

adityastic
Copy link
Collaborator

Fixes #486

Changes:

  • Moved Images to docs folder and we now copy it when we publish using fastlane

@auto-label auto-label bot added the fix label Sep 3, 2019
iamareebjamal
iamareebjamal reviewed Sep 3, 2019
View reviewed changes
scripts/upload-apk.sh Outdated

# Copy Screenshots
mkdir -p "fastlane/metadata/android/en-US/images/phoneScreenshots"
find docs -type f -name "screen-?.png" -exec cp {} "fastlane/metadata/android/en-US/images/phoneScreenshots/" \;
Copy link
Member

Choose a reason for hiding this comment

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

Just use cp

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah not using regex, glob can work even in cp. Updated

@iamareebjamal
Copy link
Member

Need to find a solution to have F-Droid work as well with this. It won't find the images in this case

@adityastic
Copy link
Collaborator Author

Need to find a solution to have F-Droid work as well with this. It won't find the images in this case

Yes true, let me look what else can I do

@adityastic
Copy link
Collaborator Author

Git logically stores each file under its SHA1. What this means is if you have two files with exactly the same content in a repository (or if you rename a file), only one copy is stored.

@iamareebjamal we should not have problem duplicating content

@iamareebjamal
Copy link
Member

Not the case with binaries. But let me check, maybe I am wrong

@iamareebjamal
Copy link
Member

Yeah, it'll work. Please go ahead.

However, please fail the build if there are different count of images in fastlane and docs folder so that when an image is added in one of them, it reminds the developer to add image in the other folder as well

@adityastic
Copy link
Collaborator Author

@iamareebjamal yeah that sounds right. Will get it done in some time

@adityastic adityastic force-pushed the patch-486 branch 7 times, most recently from 9d31d19 to c8eba92 Compare September 5, 2019 12:30
@adityastic adityastic changed the title fix: move images to docs feat: check images for fastlane and docs Sep 5, 2019
@auto-label auto-label bot added feature and removed fix labels Sep 5, 2019
@adityastic
Copy link
Collaborator Author

adityastic commented Sep 5, 2019 •
edited
Loading

@iamareebjamal I did not opt to check the count rather I check the md5 which will give us the exact content duplication difference.

Failing Build Result: https://travis-ci.com/fossasia/badge-magic-android/builds/126048064
Passing Build Result: https://travis-ci.com/fossasia/badge-magic-android/jobs/231440485

@adityastic adityastic changed the title feat: check images for fastlane and docs chore: check images for fastlane and docs Sep 5, 2019
@auto-label auto-label bot added chore and removed feature labels Sep 5, 2019
@adityastic adityastic self-assigned this Sep 5, 2019
iamareebjamal
iamareebjamal reviewed Sep 5, 2019
View reviewed changes
scripts/check-screenshots.sh Outdated
FILES_DIFF=$(diff <(find docs/images -type f -name "screen-?.png" -exec md5sum {} + | sort -k 2 | sed 's/ .*\// /') <(find fastlane/metadata/android/en-US/images/phoneScreenshots -type f -exec md5sum {} + | sort -k 2 | sed 's/ .*\// /'))
if [[ $FILES_DIFF ]]; then
echo -e "\033[0;31mScreenshots in docs/images and fastlane/metadata/android/en-US/images/phoneScreenshots are not same\033[0m" >&2
exit 5;
Copy link
Member

Choose a reason for hiding this comment

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

Why not exit 1?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes 1 is for general errors, we can use that as well. I just wanted to make it unique to our application so I chose a random 5

Copy link
Member

Choose a reason for hiding this comment

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

I think it is better to use standard errors. CI may be hardcoded to understand only 1 exit code

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@iamareebjamal yes true, updated with exit 1

@iamareebjamal iamareebjamal merged commit b2a35d0 into fossasia:development Sep 6, 2019
@adityastic adityastic deleted the patch-486 branch September 15, 2019 12:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iamareebjamal iamareebjamal iamareebjamal approved these changes

@mariobehling mariobehling Awaiting requested review from mariobehling

@yashk2000 yashk2000 Awaiting requested review from yashk2000

Assignees

@adityastic adityastic

Labels
chore
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Readme.md: Add screenshots to /docs/images and add them into the Readme
2 participants
@adityastic @iamareebjamal