-
Notifications
You must be signed in to change notification settings - Fork 90
Add dependency update PR exception for changelog test #723
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
Conversation
Codecov Report
@@ Coverage Diff @@
## master #723 +/- ##
=======================================
Coverage 99.21% 99.21%
=======================================
Files 140 140
Lines 4985 4985
=======================================
Hits 4946 4946
Misses 39 39 Continue to review full report at Codecov.
|
@@ -193,14 +193,18 @@ jobs: | |||
- run: | # debugging info | |||
if [[ $(expr match "${CIRCLE_BRANCH}" "release_v[0-9.]\+") -gt 0 ]]; then | |||
echo This is a release PR; | |||
elif [[ "${CIRCLE_BRANCH}" = "dep-update" ]]; then |
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.
Got it, so we're special-casing this branch? That seems fine to me.
Is there any documentation we should add to this, perhaps in contributing.md
? Not sure if there's a good place for that right now. Maybe unnecessary
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 seems like a good place for now!
else | ||
echo This is not a release PR; | ||
echo This is a regular 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.
Ah, I had forgotten I introduced this pattern of duplicating this code across debug vs actual blocks.
Now that this is growing in complexity, could we move these echo
statements down into the lower if/elif/else
?
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.
Left two comments but otherwise LGTM!
Fixes #680. Test with current branch.