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

ci(danger): Do not require changelogs from dependabot #714

Merged
merged 4 commits into from
Apr 11, 2022

Conversation

flub
Copy link
Contributor

@flub flub commented Mar 9, 2022

For plain dependency upgrades that require not changes we normally do
not need any changelog entries. But not having them fails CI which
renders every dependabot PR a huge hassle.

Sadly we can not customise the dependabot PRs to add a #skip-changelog
marker (see e.g.
dependabot/dependabot-core#2091), so lets
resort to changing the check instead.

There is a risk that someone pushes some changes to such a PR that
does require a changelog. Hopefully we'll recognise this and do the
right thing anyway.

#skip-changelog

For plain dependency upgrades that require not changes we normally do
not need any changelog entries.  But not having them fails CI which
renders every dependabot PR a huge hassle.

Sadly we can not customise the dependabot PRs to add a #skip-changelog
marker (see e.g.
dependabot/dependabot-core#2091), so lets
resort to changing the check instead.

There is a risk that someone pushes some changes to such a PR that
does require a changelog.  Hopefully we'll recognise this and do the
right thing anyway.
@flub flub requested review from a team and jan-auer March 9, 2022 11:07
@jan-auer
Copy link
Member

jan-auer commented Mar 9, 2022

I would like not to have this. We need to review & approve every dependency bump anyway, and as part of that can add a #skip-changelog to the PR description. If dependabot overwrites the description, we should rather update our changelog bot to also check in comments.

Now the actual reason why I would like to have this: In some cases we do want a changelog entry and it's too easy to forget. Especially for dependency upgrades that fix CVEs, come with user-visible changes, or in the case of symbolic (we're setting a precedent here) require downstream changes.

@flub
Copy link
Contributor Author

flub commented Mar 9, 2022

So I describe the tradeoff made here pretty clearly: if you don't need to push any changes to the PR you won't need a changelog. But if you do make changes it won't prompt you.

I think I could like the suggestion of looking in comments as well though, it would mean you can write:

@dependabot merge
#skip-changelog

and it'll just work. let me change it to that to try it out for a while

@jan-auer
Copy link
Member

jan-auer commented Mar 9, 2022

The comment compromise sounds pretty sweet, if dependabot is graceful with the delay. Otherwise, we just comment #skip-changelog, and then merge with the large green button at the bottom.

Counter example for checking for changes: getsentry/relay#1207
In this case we just bumped the dependency, but it also fixed a CVE so it's significant enough to communicate.

Look for #skip-changelog markers in the review comments instead.
@flub flub requested a review from Swatinem March 9, 2022 11:30
@flub
Copy link
Contributor Author

flub commented Mar 9, 2022

PTAL

@jan-auer
Copy link
Member

jan-auer commented Mar 9, 2022

We may have a problem now. Danger checks for a comment with #skip-changelog, but it's own comment that it leaves on the PR contains this string...

(NB I'm not sure if reviews actually includes all comments or just "comment" reviews)

@codecov-commenter
Copy link

codecov-commenter commented Mar 9, 2022

Codecov Report

Merging #714 (6f11f86) into master (ef06513) will decrease coverage by 0.41%.
The diff coverage is 40.73%.

❗ Current head 6f11f86 differs from pull request most recent head cd8c4d2. Consider uploading reports for the commit cd8c4d2 to get more accurate results

@@            Coverage Diff             @@
##           master     #714      +/-   ##
==========================================
- Coverage   73.63%   73.22%   -0.42%     
==========================================
  Files          48       48              
  Lines       10698    10835     +137     
==========================================
+ Hits         7878     7934      +56     
- Misses       2820     2901      +81     
Impacted Files Coverage Δ
crates/process-event/src/main.rs 1.12% <0.00%> (+0.05%) ⬆️
crates/symbolicator/src/services/cficaches.rs 87.14% <ø> (ø)
crates/symbolicator/src/services/download/s3.rs 41.45% <ø> (+0.34%) ⬆️
crates/symbolicator/src/services/minidump.rs 93.12% <0.00%> (ø)
...tes/symbolicator/src/services/symbolication/mod.rs 71.35% <6.69%> (-3.33%) ⬇️
crates/symbolicator/src/types/mod.rs 55.75% <28.57%> (-1.33%) ⬇️
crates/symbolicator/src/test.rs 84.78% <50.00%> (-0.44%) ⬇️
...icator/src/services/symbolication/module_lookup.rs 93.12% <98.43%> (+1.83%) ⬆️
... and 3 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 ef06513...cd8c4d2. Read the comment docs.

@relaxolotl
Copy link
Contributor

relaxolotl commented Mar 22, 2022

We may have a problem now. Danger checks for a comment with #skip-changelog, but it's own comment that it leaves on the PR contains this string...

(NB I'm not sure if reviews actually includes all comments or just "comment" reviews)

FWIW the code in the PR currently checks top-level comments included in a review. Let's use #673 as an example: the review loop iterates over the entries returned by https://api.github.com/repos/getsentry/symbolicator/pulls/673/reviews.

If you want to look at all comments, the endpoint (using the above PR) is actually https://api.github.com/repos/getsentry/symbolicator/issues/673/comments. If the intent is to scan through all comments, you probably want to invoke danger.github.api.getPullRequestComments. The API also has danger.github.api.getDangerCommentIDs which you can use to ignore danger's comments and therefore dodge this issue, assuming that's publicly exposed.

Also need to give this permission to write to the pull request again,
seems like our org settings have been tightened and this could no
longer write to the PR.
@flub
Copy link
Contributor Author

flub commented Apr 6, 2022

PTAL, this now only accepts #skip-changelog in comments that also include a dependabot command. This should avoid our loop.

@flub flub requested a review from relaxolotl April 8, 2022 12:14
@flub flub merged commit b8bd1ea into master Apr 11, 2022
@flub flub deleted the flub/danger-dependabot-ok branch April 11, 2022 11:29
flub added a commit that referenced this pull request Apr 11, 2022
flub pushed a commit that referenced this pull request Apr 11, 2022
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.

5 participants