-
Notifications
You must be signed in to change notification settings - Fork 74
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
add dependabot.yml #2353
base: main
Are you sure you want to change the base?
add dependabot.yml #2353
Conversation
🦋 Changeset detectedLatest commit: a79bfee The changes in this PR will be included in the next version bump. This PR includes changesets to release 0 packagesWhen changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Co-authored-by: Kamil Sobol <[email protected]>
# Runs every Monday | ||
interval: 'weekly' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why weekly and not say daily?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking weekly to start so we aren't constantly bombarded with version update PRs, especially since we are using increase-if-necessary
the PRs will most likely be for new MVs so we need to be thorough on checking for breaking changes.
Or we make sure E2E runs on these PRs and we assume if E2E passes then there are no breaks, in that case I'm fine with changing it to daily.
# Runs every Monday | ||
interval: 'weekly' | ||
# Update package.json files if new version is outside of version range specified there. Otherwise lock file only. | ||
versioning-strategy: increase-if-necessary |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this strategy can update package.json files, how will we accommodate changesets
in those PRs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point. Best if this could be automated.
If it can't:
PR won't pass checks until changeset is added (if our check works)
Maintainer adds changeset (this is doable using UI here #2353 (comment) +/- quotes style but we could exclude changeset files from prettier.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point on changesets
, I'll see if I can automate it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With change in d826612, I used the new script locally for this PR rtpascual#20 and this is the resulting changeset file that will be force pushed to the PR (had to do it locally to simulate health_checks running on PR):
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea for automation looks good.
.github/dependabot.yml
Outdated
# Update package.json files if new version is outside of version range specified there. Otherwise lock file only. | ||
versioning-strategy: increase-if-necessary | ||
labels: | ||
- 'dependency' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the purpose of dependency
label?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just keeping that label from past dependabot PRs, I'll remove the labels here since we will be adding it through the script
const getVersionUpdates = async (files: string[]) => { | ||
const updates = new Map<string, string>(); | ||
for (const file of files) { | ||
const changes = await gitClient.getFileChanges(file); | ||
for (const change of changes.split(EOL)) { | ||
if (!change.startsWith('+ ')) { | ||
continue; | ||
} | ||
// This will capture lines in git diff like: | ||
// + "<dependency>": "^<version>" | ||
const match = change.match(/"(.*)": "(.*)"/); | ||
|
||
if (!match) { | ||
continue; | ||
} | ||
// Add dependency name (match[1]) and new version (match[2]) to Map | ||
updates.set(match[1].replace(/"/g, ''), match[2].replace(/"/g, '')); | ||
} | ||
} | ||
|
||
return updates; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One other source to consider is parse PR title or description. I'm not sure if this will be any simple - please evaluate and adopt or reject this suggestion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sometimes dependabot will group version updates together so the title can be something like Bump the npm_and_yarn group across 1 directory with 3 updates
.
I think description will be best, since it gives us the version dependabot updated from since there will be a line like Updates/Bumps <dependency> from <old-version> to <new-version>
.
// Get modified package.json files that we care about for changesets | ||
const packageJsonFiles = changedFiles.filter( | ||
(changedFile) => | ||
changedFile.startsWith('packages/') && | ||
!['packages/integration-tests', 'packages/eslint-rules'].some( | ||
(packageName) => changedFile.startsWith(packageName) | ||
) && | ||
changedFile.endsWith('package.json') | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use private
instead of names, see
amplify-backend/scripts/components/api-changes-validator/api_changes_validator.ts
Line 49 in b574bce
if (latestPackageJson.private) { |
await gitClient.commitAllChanges('add changeset'); | ||
await gitClient.push({ force: true }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does the stop condition work?
I.e. this will presumably push a change to PR and trigger new workflow. Would that trigger this script again or not ? If this is handled already - please drop a comment somewhere - it's not easy to catch this detail.
Other thing is e2e tests.
In case script adds extra commit then we'll be kicking off 2 e2e runs next to each other. If possible we should kick off one.
For example this script could add run-e2e label before pushing. And always commit something (even if it's only pacakge-lock update).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch I don't have anything in place to stop pushing the changeset from triggering the script again, I'll see where I can handle that (possibly using the fact that the script may be adding the run-e2e
label?).
That's a good idea on the script adding the run-e2e
label to prevent the initial e2e run, then we can push an empty commit or empty changeset (in case of only package-lock update).
@@ -0,0 +1,93 @@ | |||
import fsp from 'fs/promises'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rename this script to dependabot_handle_version_update
or something like this.
It's doing a bit more than generating changeset (i.e. it's also pushing it to repo).
Also - consider writing a happy case test case for this.
Something like https://github.com/aws-amplify/amplify-backend/blob/main/scripts/components/release_lifecycle.test.ts .
Problem
We can be more proactive in keeping our dependencies up to date so we can avoid problem statements like in #2303.
Issue number, if available:
Changes
Adds
dependabot.yml
which will trigger Dependabot to check for version updates of our dependencies.Corresponding docs PR, if applicable:
Validation
Forked the repo and used
dependabot.yml
to create PRs to update dependencies, see https://github.com/rtpascual/amplify-backend-fork/pulls.Checklist
run-e2e
label set.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.