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

fix(lfs-detection): take into account files without patch #138

Merged
merged 2 commits into from
Jun 30, 2022
Merged

fix(lfs-detection): take into account files without patch #138

merged 2 commits into from
Jun 30, 2022

Conversation

davidlj95
Copy link
Contributor

@davidlj95 davidlj95 commented Apr 12, 2022

Description

Fix #135 issue with PDF files.

Issue

For PDF files, seems that the patch field from the PR list files API is not there. Therefore, the check to see if it is stored in LFS or not fails

lfs-warning/src/index.ts

Lines 53 to 55 in 5d972cf

const isStoredInLFS = Boolean(
file.patch?.includes('version https://git-lfs.github.com/spec/v1')
);

Proposed solution

Use the content field from the get blob API call for those files without patch. If file contents (base64 encoded, as per docs) start with the Git LFS usual file contents (version https://git-lfs...) then it's very probably properly stored in Git LFS

Tests

@davidlj95 davidlj95 requested a review from ppremk as a code owner April 12, 2022 23:40
@davidlj95
Copy link
Contributor Author

❓ Unsure about why diff in index.js is so big 🤔 I just did npm run package to generate it. Maybe because using Buffer?

src/index.ts Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
@ppremk
Copy link
Owner

ppremk commented Apr 19, 2022

👋🏽 @davidlj95 @tsimbalar thank you for the contribution 💖 Apologies for not getting back ASAP. May I suggest that we resolve the conversations before the approval please? This will allow for a recorded audit on the decision we had taken for folks who may be following along. Also please ping me in here once it is resolved. I will be able to do the next steps 🙇🏽

@ppremk
Copy link
Owner

ppremk commented May 23, 2022

@davidlj95 @tsimbalar is there anything I can help to get this moving? 🙂

@davidlj95
Copy link
Contributor Author

@davidlj95 @tsimbalar is there anything I can help to get this moving? 🙂

Hi @ppremk ! Thanks to you 😊

No worries, as you've seen, wasn't something urgent anyway :P Sorry for disconnecting about this a bit.

We just talked and resolved conversations. Will rebase, generate the new index.js and I think it will be ready to go 🚀

@davidlj95
Copy link
Contributor Author

☝️ Done in b3077e6

Copy link
Owner

@ppremk ppremk left a comment

Choose a reason for hiding this comment

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

@davidlj95 Let's :shipit: feel free to merge and ping me. I will proceed to cut a new release

@tsimbalar
Copy link

@davidlj95 can you merge it :P :P

@ppremk ppremk merged commit c8336a0 into ppremk:master Jun 30, 2022
@ppremk
Copy link
Owner

ppremk commented Jun 30, 2022

Merging on behalf

@tsimbalar
Copy link

Merging on behalf

Thanks @ppremk 👍

What is the next step for this change to be part of the next release ? :)

@davidlj95 davidlj95 deleted the fix/consider-files-with-no-patch branch July 2, 2022 04:01
@ppremk
Copy link
Owner

ppremk commented Jul 11, 2022

@tsimbalar forgive me for missing out on this notification. Was OOO and travelling. I have blocked time to cut a new a release and add some release notes pointing to this Issue/PR. 🙇🏽 Thank you for your patience and continued support.

@tsimbalar
Copy link

Hi @ppremk is there any chance you can publish a release in the near future ? :)

@ppremk
Copy link
Owner

ppremk commented Aug 29, 2022

Hi @ppremk is there any chance you can publish a release in the near future ? :)

@tsimbalar thank you for the ping. It is now published. Please pardon the delay, this is on me for not getting to it on time. Please let know if there is anything else pending. 🙇🏽

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants