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

Correct PR body comparison #996

Merged
merged 1 commit into from Nov 15, 2021

Conversation

CasperWA
Copy link
Member

Fixes #995

Only compare the first 8 lines of the PR body when deciding whether the
permanent dependabot branch should be reset to master or have master
merged into it.

I've tested this locally, retrieving the PR body for PR #993 and it seems to work now.

Only compare the first 8 lines of the PR body when deciding whether the
permanent dependabot branch should be reset to `master` or have `master`
merged into it.
@codecov
Copy link

codecov bot commented Nov 10, 2021

Codecov Report

Merging #996 (95067c8) into master (8065f6f) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #996   +/-   ##
=======================================
  Coverage   92.91%   92.91%           
=======================================
  Files          67       67           
  Lines        3780     3780           
=======================================
  Hits         3512     3512           
  Misses        268      268           
Flag Coverage Δ
project 92.91% <ø> (ø)
validator 92.91% <ø> (ø)

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


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8065f6f...95067c8. Read the comment docs.

@JPBergsma
Copy link
Contributor

JPBergsma commented Nov 11, 2021

I am not sure, I understand how this code works, but is it always guaranteed that the first 8 lines do not contain a checkbox?
Is the comparison only between the last PR in each branch? I only saw one PR in my "single_dependency_pr_body.txt". In that case you can be certain that if a PR contains a checkbox, the last update was not a Dependabot update, so the result should be false and there is no problem.

@CasperWA
Copy link
Member Author

I am not sure, I understand how this code works, but is it always guaranteed that the first 8 lines do not contain a checkbox? (...)

The file being checked is this file.
It's not fool-proof for sure. If someone changes the created PR's body, specifically the first 8 lines, then this will obviously fail. But the first checkbox starts on line 9.

Is the comparison only between the last PR in each branch? I only saw one PR in my "single_dependency_pr_body.txt". In that case you can be certain that if a PR contains a checkbox, the last update was not a Dependabot update, so the result should be false and there is no problem.

I think that's too general of a constraint to make (checking for checkboxes, as we sometimes add checkboxes in other PR bodies as well).
It checks the latest PR that has been merged - since the workflow runs every time a new commit is pushed to master, it checks the latest PR that has been merged into master. Hence, it's effectively checking the PR body of the PR that has just been merged.
There is a caveat here, of course, namely that if someone merges several PRs in succession, then this will fail as the workflow might not read the PR body of its associated merge that caused the workflow to run to begin with.
Don't know if that made sense... I tried 😅

@CasperWA
Copy link
Member Author

@JPBergsma @ml-evs can we get this in before Wednesday?

Copy link
Member

@ml-evs ml-evs left a comment

Choose a reason for hiding this comment

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

lgtm

@CasperWA CasperWA merged commit 68de947 into master Nov 15, 2021
@CasperWA CasperWA deleted the cwa/fix-995-reset-permanent-dependencies-branch branch November 15, 2021 11:48
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.

Updating permanent dependabot branch not working after updating dependencies
3 participants