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

v3 #124

Merged
merged 7 commits into from
Oct 20, 2021
Merged

v3 #124

merged 7 commits into from
Oct 20, 2021

Conversation

naseemkullah
Copy link
Collaborator

@naseemkullah naseemkullah commented Aug 2, 2021

closes #106
closes #123
closes #125
closes #105

  • use https://www.npmjs.com/package/gts to offload all TS/eslint/prettier config

  • lint, rename vars

  • feat: add detection of files that are lfs tracked but were accidentally checked in

  • feat: remove label if check passes on a previously failing PR (files were stored in LFS or removed)

  • feat: add file exclusion option

  • feat: accept units of measurement (b, mb or gb) in a non breaking manner (no unit implies bytes)

@naseemkullah naseemkullah requested a review from ppremk as a code owner August 2, 2021 23:25
@naseemkullah naseemkullah changed the title conver to TS convert to TS Aug 2, 2021
@naseemkullah naseemkullah force-pushed the ts branch 2 times, most recently from da063ad to d7fd126 Compare August 2, 2021 23:54
@naseemkullah naseemkullah changed the title convert to TS convert to TS + prevent accidentally checking in LFS files Aug 3, 2021
@ppremk
Copy link
Owner

ppremk commented Aug 4, 2021

have block sometime in the coming week to review. Please anticipate some delays. If we merge this then we will need to publish a new release. 🙇🏽

@naseemkullah
Copy link
Collaborator Author

Thanks @ppremk 🙏

@naseemkullah naseemkullah changed the title convert to TS + prevent accidentally checking in LFS files convert to TS + prevent accidentally checking in LFS files + remove label if check passes Aug 5, 2021
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@ppremk
Copy link
Owner

ppremk commented Aug 8, 2021

@naseemkullah thank you for your patience 🙇🏽 Some feedbacks along with suggested changes

  • Some final prep before we can merge this.. internally we have recommendation to use ncc over esbuild. For me this is not a show stopper, just wondering if there are any specifics to go with ncc?
  • Next steps would be to prepare a release note for the the new version we are going to be publishing. It should be a new v3.0 since we are moving away from js along with the new features shipped with it
  • We should start thinking about adding some test (I am guilty as charged for not writing those to begin with, this started as a rough idea which is catching up.. ❤️ )
  • I will also update the README.md with the latest changes

What are your thoughts on this? 🙂

@naseemkullah
Copy link
Collaborator Author

@naseemkullah thank you for your patience 🙇🏽 Some feedbacks along with suggested changes

* Some final prep before we can merge this.. internally we have recommendation to use `ncc` over `esbuild`. For me this is not a show stopper, just wondering if there are any specifics to go with `ncc`?

* Next steps would be to prepare a release note for the the new version we are going to be publishing. It should be a new v3.0 since we are moving away from `js` along with the new features shipped with it

* We should start thinking about adding some test (I am guilty as charged for not writing those to begin with, this started as a rough idea which is catching up.. ❤️ )

* I will also update the README.md with the latest changes

What are your thoughts on this? 🙂

Thanks for taking a look @ppremk !

It all sounds good, though it would be nice to add file exclusion feature as well when releasing 3.0.0 should I add that feature in this PR or can it be done after?

re: release notes, should I add these?

use git check-attr to determine if any PR files should be stored in LFS.

Then among the PR files that should be stored in LFS (if any), check for
the string version https://git-lfs.github.com/spec/v1 to determine if the
file is indeed a pointer or not.
@ppremk
Copy link
Owner

ppremk commented Aug 12, 2021

t all sounds good, though it would be nice to add file exclusion feature as well when releasing 3.0.0 should I add that feature in this PR or can it be done after?

re: release notes, should I add these?

@naseemkullah If you have bandwidth we can tackle this with the same PR. I have not dropped the ball on this, will finalize hopefully by next week. Waiting on feedback from our internal team for the final go ahead. Thanks for your patience and understanding 🙇🏽

@naseemkullah
Copy link
Collaborator Author

naseemkullah commented Aug 15, 2021

I've added the exclusionPatterns feature now @ppremk

I've also implemented a feat to accept units of measurement (b, mb or gb) in a non breaking manner (no unit implies bytes)

I will attach some files and edit the workflow in this repo in the coming days to run a few tests on the code within this PR.

@naseemkullah naseemkullah changed the title convert to TS + prevent accidentally checking in LFS files + remove label if check passes v3 Aug 16, 2021
@ppremk
Copy link
Owner

ppremk commented Aug 16, 2021

@naseemkullah I've added some changes to the README.md. Let me know when your test is complete then we proceed with the merge and I will publish a new release 🙂

@naseemkullah
Copy link
Collaborator Author

Hi @ppremk when you get a chance please approve https://github.com/ActionsDesk/lfs-warning/actions/runs/1139191774 to test the workflow 🙏

@naseemkullah
Copy link
Collaborator Author

HI @ppremk https://github.com/ActionsDesk/lfs-warning/actions/runs/1146752721 🙏

Or could you temporarily enable workflows while I iron out the kinks?

@ppremk
Copy link
Owner

ppremk commented Aug 19, 2021

@naseemkullah if this is churns out ✅ I will proceed to publish a new release. Feel free to merge once the test runs are completed successfully. I will also check with the organization owners to see if we can directly add you as outside collaborator for this repo, to keep things moving 🙂

@naseemkullah
Copy link
Collaborator Author

@naseemkullah if this is churns out ✅ I will proceed to publish a new release. Feel free to merge once the test runs are completed successfully. I will also check with the organization owners to see if we can directly add you as outside collaborator for this repo, to keep things moving 🙂

Thanks @ppremk :)
BTW there still seems to be some bugs (even on the last run that resulted in a pass 😅 )

@ppremk
Copy link
Owner

ppremk commented Aug 19, 2021

@naseemkullah ok.. I will wait for your explicit Go ahead before approving to merge. Is that ok?

@naseemkullah
Copy link
Collaborator Author

That sounds good @ppremk, in the meantime can you please enable workflows to run upon my commits, or just keep approving them, I tried making a PR to my own branch but no workflow was triggered: naseemkullah#2

@naseemkullah
Copy link
Collaborator Author

It's getting close to ready, though I cannot test further due to Error: Resource not accessible by integration error which according actions/first-interaction#10 is because i do not have permissions on this repo.

Is there anyway to give me more rights on this repo @ppremk, if not are you willing to release with potential bugs that we would fix in a subsequent patch release?

accept unit of measurement (b,mb,gb) for filesizelimit input

also:

add no-floating-promises rule

move try/catch to top level

factor out common props for issue rest calls

extract functions to improve readability

add label and create comment in parallel

use execFile instead of exec to properly escape filename
@naseemkullah
Copy link
Collaborator Author

@ppremk if I cannot get rights to the repo (so that a workflow triggered by me can add the label to the PR), let's just merge, as far as I can test this is good to go!

@ppremk
Copy link
Owner

ppremk commented Aug 25, 2021

@naseemkullah unfortunately I am not the owner of the org, I will work again with the owner to explore how we can move forward with the correct access rights for collaborators.

Since this round of the test has passed. I will proceed to merge (in a day or two) first and cut a release soon after. I plan to do this simultaneously thus the reason for not merging yet.

@naseemkullah
Copy link
Collaborator Author

Thanks @ppremk

@ppremk
Copy link
Owner

ppremk commented Sep 3, 2021

still in my radar 😓

@ppremk
Copy link
Owner

ppremk commented Sep 27, 2021

@naseemkullah I have not forgotten about this. Have been rushing for project go live thus its taking longer. Please bear with me.. and thank you for your understanding 🙇🏽

@naseemkullah
Copy link
Collaborator Author

Thanks for the update @ppremk and good luck with project go live! This PR will be waiting when you're ready 😄

@ppremk
Copy link
Owner

ppremk commented Oct 20, 2021

@naseemkullah let's 🚢 this. It has been delayed on my part. Thank you for your patience and understanding. I appreciate it. Let's push the merge and I will start publishing a new release of this. Also what is your LinkedIn profile? I would like to broadcast this new release in there too. 🙂 (with your permission to mention your contribution)

@naseemkullah
Copy link
Collaborator Author

@naseemkullah let's 🚢 this. It has been delayed on my part. Thank you for your patience and understanding. I appreciate it. Let's push the merge and I will start publishing a new release of this. Also what is your LinkedIn profile? I would like to broadcast this new release in there too. 🙂 (with your permission to mention your contribution)

Great, thanks! 😊
https://www.linkedin.com/in/naseemkullah

@ppremk ppremk merged commit 811b797 into ppremk:master Oct 20, 2021
@ppremk
Copy link
Owner

ppremk commented Oct 20, 2021

@naseemkullah the latest Release v3.0 is now published in the marketplace. Thank you for your contribution. Feel free to give it a spin and report back if we need change anything. ❤️ 🎉

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