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

[No BZ] Warn developers if they upgrade a dependency but not a patch #36776

Closed
roryabraham opened this issue Feb 18, 2024 · 12 comments
Closed

[No BZ] Warn developers if they upgrade a dependency but not a patch #36776

roryabraham opened this issue Feb 18, 2024 · 12 comments
Assignees
Labels
FirstPick Engineering only, please! Only add when there is an identified code solution. Internal Requires API changes or must be handled by Expensify staff Monthly KSv2 NewFeature Something to build that is a new item. Not a priority Reviewing Has a PR in review

Comments

@roryabraham
Copy link
Contributor

roryabraham commented Feb 18, 2024

Coming from #36775...

Problem

When upgrading dependencies, it's possible that you may break a patch for that dependency. It's also possible that the patch may no longer be needed. It's easy to forget this, which leaves unused patch files in our repo.

Solution

Add a simple PR check to our webhook that looks for PRs that upgrade a dependency with a patch without updating the patch and include a PR comment like:

Heads up! It looks like you updated a dependency without updating the patch. Do you need to update the patch?

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~019999ca1782dc1db6
  • Upwork Job ID: 1759309916923375616
  • Last Price Increase: 2024-02-18
@roryabraham roryabraham added Monthly KSv2 NewFeature Something to build that is a new item. labels Feb 18, 2024
@roryabraham roryabraham self-assigned this Feb 18, 2024
Copy link

melvin-bot bot commented Feb 18, 2024

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Monthly KSv2 labels Feb 18, 2024
@roryabraham roryabraham added the Internal Requires API changes or must be handled by Expensify staff label Feb 18, 2024
Copy link

melvin-bot bot commented Feb 18, 2024

Job added to Upwork: https://www.upwork.com/jobs/~019999ca1782dc1db6

Copy link

melvin-bot bot commented Feb 18, 2024

Triggered auto assignment to Contributor Plus for review of internal employee PR - @rushatgabhane (Internal)

@roryabraham
Copy link
Contributor Author

I was thinking this should be internal like other similar-style warnings in our webhook. Don't need a C+, not sure why we assign them for issues labelled Internal...

@roryabraham
Copy link
Contributor Author

Some notes:

  • We should split up commentIfPatchFileDiffers into commentIfPackageJsonUpdated and commentIfComposerJsonUpdated
  • We should create constants for REPOS_WITH_PACKAGE_JSON and REPOS_WITH_COMPOSER_JSON
  • We need to do some json parsing of the package.json diff to see which dependencies were updated so we can check for patches in that specific dependency

@roryabraham
Copy link
Contributor Author

No near-term plans to push this DX improvement forward. Going to drop for focus

@roryabraham roryabraham removed their assignment Mar 13, 2024
@roryabraham roryabraham added the FirstPick Engineering only, please! Only add when there is an identified code solution. label Mar 13, 2024
@mallenexpensify mallenexpensify changed the title Warn developers if they upgrade a dependency but not a patch [No BZ] Warn developers if they upgrade a dependency but not a patch Apr 4, 2024
@mallenexpensify
Copy link
Contributor

Updated title to denote this NewFeature doesn't need a BZ assigned.

@roryabraham
Copy link
Contributor Author

This also isn't really in the roadmap anywhere - it's more of an automation to prevent developer mistakes

@melvin-bot melvin-bot bot closed this as completed Jun 27, 2024
Copy link

melvin-bot bot commented Jun 27, 2024

@roryabraham, this Monthly task hasn't been acted upon in 6 weeks; closing.

If you disagree, feel encouraged to reopen it -- but pick your least important issue to close instead.

@roryabraham
Copy link
Contributor Author

roryabraham commented Jun 28, 2024

This was quick and easy to implement, and failing to correctly update our patches can lead to headaches. So I created a PR to add this check. It's all-too-common for patches to be added with limited context and then basically left forever, so hopefully this should help curb that practice.

@roryabraham roryabraham self-assigned this Jun 28, 2024
@roryabraham roryabraham added the Reviewing Has a PR in review label Jun 28, 2024
Copy link

melvin-bot bot commented Aug 26, 2024

@roryabraham, this Monthly task hasn't been acted upon in 6 weeks; closing.

If you disagree, feel encouraged to reopen it -- but pick your least important issue to close instead.

@melvin-bot melvin-bot bot closed this as completed Aug 26, 2024
@roryabraham
Copy link
Contributor Author

oh, this has been done for a while 🙈

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FirstPick Engineering only, please! Only add when there is an identified code solution. Internal Requires API changes or must be handled by Expensify staff Monthly KSv2 NewFeature Something to build that is a new item. Not a priority Reviewing Has a PR in review
Projects
None yet
Development

No branches or pull requests

4 participants