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

feat: enable dependabot for GHA and frontend packages #6424

Merged
merged 3 commits into from
Jan 8, 2024

Conversation

Bento007
Copy link
Contributor

@Bento007 Bento007 commented Jan 5, 2024

Reason for Change

  • dependabot will automatically check for package updates and open pull request to update them.
  • it will also make recommendation when a security vulnerability is discovered in a package we depend on.

Changes

  • add dependabot.yml to check for updated to GHA and frontend packages.

Copy link
Contributor

github-actions bot commented Jan 5, 2024

Deployment Summary

Copy link

codecov bot commented Jan 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (59c3b49) 92.51% compared to head (f4234b1) 92.42%.

❗ Current head f4234b1 differs from pull request most recent head 02f6d90. Consider uploading reports for the commit 02f6d90 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6424      +/-   ##
==========================================
- Coverage   92.51%   92.42%   -0.09%     
==========================================
  Files         179      179              
  Lines       14783    14702      -81     
==========================================
- Hits        13676    13588      -88     
- Misses       1107     1114       +7     
Flag Coverage Δ
unittests 92.42% <ø> (-0.09%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@prathapsridharan
Copy link
Contributor

I have a few questions:

  1. What happens if there are many such pull requests for updated packages?
  2. Will engineers actually go through each PR and consider whether to update or not?
  3. What if engineers ignore them and stale package update PRs build up?
  4. Is it possible that an old update for package A was missed (due to an ignored PR) but then a PR for update package B which depends on an updated version of package A is opened and the reviewer accepts the package B update? Would that cause a failure in the build?

@Bento007
Copy link
Contributor Author

Bento007 commented Jan 5, 2024

@prathapsridharan

  1. we can configure how many dependabot will have open at a time. The default is 5 per requirements file. It will give us a change to gradually burn down the backlog of requirements without being swamped.
  2. If there are no errors from our automated tests, I don't see a strong reason to not accept the PR. If errors occur we can decide how to proceed.
  3. We have a reaper that closes stale PRs after two weeks if they are not resolved.
  4. In that case we should pin package A to a version so B does not try to update. We should also see our tests fails as a result. This is something the package manager poetry is helpful at preventing in python.

@prathapsridharan
Copy link
Contributor

@prathapsridharan

  1. we can configure how many dependabot will have open at a time. The default is 5 per requirements file. It will give us a change to gradually burn down the backlog of requirements without being swamped.
  2. If there are no errors from our automated tests, I don't see a strong reason to not accept the PR. If errors occur we can decide how to proceed.
  3. We have a reaper that closes stale PRs after two weeks if they are not resolved.
  4. In that case we should pin package A to a version so B does not try to update. We should also see our tests fails as a result. This is something the package manager poetry is helpful at preventing in python.

For (4), in the scenario I have in mind, the dependabot PR to update package A did not happen due to an engineer forgetting about the PR and the reaper closing that PR. So for example, package A was at version 1.0 and did not get updated to 1.1 by involuntary omission. Now, package B update PR comes but it depends on package A being at 1.1 which didn't happen. But an engineer accepted this PR to update package B which might then cause a failed build right?

I don't know how likely this scenario is and whether it is worth agonizing over. Just want to point it out because this does seem to rely on some level of developer diligence but it might be manageable if there aren't too many PRs coming in.

@Bento007
Copy link
Contributor Author

Bento007 commented Jan 6, 2024

@prathapsridharan

  1. we can configure how many dependabot will have open at a time. The default is 5 per requirements file. It will give us a change to gradually burn down the backlog of requirements without being swamped.
  2. If there are no errors from our automated tests, I don't see a strong reason to not accept the PR. If errors occur we can decide how to proceed.
  3. We have a reaper that closes stale PRs after two weeks if they are not resolved.
  4. In that case we should pin package A to a version so B does not try to update. We should also see our tests fails as a result. This is something the package manager poetry is helpful at preventing in python.

For (4), in the scenario I have in mind, the dependabot PR to update package A did not happen due to an engineer forgetting about the PR and the reaper closing that PR. So for example, package A was at version 1.0 and did not get updated to 1.1 by involuntary omission. Now, package B update PR comes but it depends on package A being at 1.1 which didn't happen. But an engineer accepted this PR to update package B which might then cause a failed build right?

I don't know how likely this scenario is and whether it is worth agonizing over. Just want to point it out because this does seem to rely on some level of developer diligence but it might be manageable if there aren't too many PRs coming in.

I agree it will require developer diligence for some thing. This exact scenario is why i'm in favor of switching how we manage our python dependencies to something that locks the current dependency version such a poetry. When you run poetry install it checks for these types of conflicts and raises errors.

If our tests cases don't break as a result then we have poor test coverage or it's a feature of the library we are not using. Either way it's something that can be addressed.

Starting to use dependabot will at least get us started with keeping everything in order in a more automated way that in theory should be autopilot until a breaking changes happens. We will at least have a small version gap to cover at the point if we are making use of dependabot.

@prathapsridharan
Copy link
Contributor

@Bento007 - Sounds good. Is this PR necessary since you have another PR that seems to already alert on npm packages as well as python packages?

#6425

@Bento007
Copy link
Contributor Author

Bento007 commented Jan 8, 2024

@prathapsridharan this one should be merged before #6425

@Bento007 Bento007 self-assigned this Jan 8, 2024
Copy link
Contributor

@prathapsridharan prathapsridharan left a comment

Choose a reason for hiding this comment

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

LGTM

@Bento007 Bento007 merged commit e41e51f into main Jan 8, 2024
11 checks passed
@Bento007 Bento007 deleted the tsmith/dependabot branch January 8, 2024 21:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants