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

No empty descriptions in generated changelog #13981

Merged
merged 2 commits into from Mar 24, 2017

Conversation

islemaster
Copy link
Contributor

For some reason we're seeing occasional empty PR descriptions in generated changelogs (example 1, example 2). It looks like this happens for PRs with titles "DTS (Levelbuilder > Staging)". I don't actually understand the root cause though.

In any case this makes it so we'll at least write "PR 00000" as the description in those cases, so the link is clickable, which seems like a good stopgap until we understand why those PR descriptions aren't getting picked up.

For some reason we're seeing occasional empty PR descriptions in generated changelogs ([example 1](https://github.com/code-dot-org/code-dot-org/releases/tag/v2017-03-23.0), [example 2](https://github.com/code-dot-org/code-dot-org/releases/tag/v2017-03-22.0)). It looks like this happens for PRs with titles "DTS (Levelbuilder > Staging)".  I don't actually understand the root cause though.

In any case this makes it so we'll at least write "PR 00000" as the description in those cases, so the link is clickable, which seems like a good stopgap until we understand why those PR descriptions aren't getting picked up.
Copy link
Contributor

@Hamms Hamms left a comment

Choose a reason for hiding this comment

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

LGTM! I'll be interested to see which PRs are failing; I think we filter out the various between-server PRs, so there might be something else breaking the regex.

"- [#{match['description']}](https://github.com/#{REPO}/pull/#{match['pr']})"
description = match['description']
if description.strip.empty?
description = "PR #{match['pr']}"
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I think the rubyesque way to do this would be description = "PR #{match['pr']}" if description.strip.empty?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, nice catch, I'll update.

@islemaster islemaster merged commit 7c97aa2 into staging Mar 24, 2017
@islemaster islemaster deleted the no-blank-lines-in-release-logs branch March 24, 2017 17:23
@islemaster
Copy link
Contributor Author

By 'editing' the release you can see the generated markdown for the blank lines, which is something like this:

- [](https://github.com/code-dot-org/code-dot-org/pull/13964)

I'll throw together a complete list of failure cases (there are only about ten) so we can figure this out.

@islemaster
Copy link
Contributor Author

islemaster commented Mar 24, 2017

Complete list of failures:

Release PR Title
v2017-03-23.0 #13964 DTS (Levelbuilder > Staging)
v2017-03-22.0 #13941 DTS (Levelbuilder > Staging)
v2017-03-21.0 #13911 DTS (Levelbuilder > Staging)
v2017-03-20.1 #13877 DTS (Levelbuilder > Staging)
v2017-03-17.0 #13854 DTS (Levelbuilder > Staging)
v2017-03-16.0 #13830 DTS (Levelbuilder > Staging)
v2017-03-15.0 #13808 DTS (Levelbuilder > Staging)
v2017-03-15.0 #13795 DTS (Levelbuilder > Staging)
v2017-03-14.0 #13785 DTS (Levelbuilder > Staging)
v2017-03-13.1 #13764 DTS (Levelbuilder > Staging)
v2017-03-10.0 #13723 DTS (Levelbuilder > Staging)
v2017-03-09.0 #13698 DTS (Levelbuilder > Staging)

Well, that's conclusive. In releases earlier than this I believe the PRs were just named "Levelbuilder > Staging". Now to work out what the bad interaction with the regex is? The parens seem like a likely candidate.

@Hamms
Copy link
Contributor

Hamms commented Mar 24, 2017

Ah, yeah, Asher's DotD script updates changed the naming scheme. Still, seems strange that this wouldn't work; the regex is pretty permissive.

@islemaster
Copy link
Contributor Author

Interesting, the problem isn't the regex at all:

git log --merges --oneline production^1..production^2 --format="%s %b"

Isn't including the PR title in the merge commit description for DTLs anymore. Any ideas?

Merge pull request #13965 from code-dot-org/staging 
Merge pull request #13962 from code-dot-org/fixScript Fix script (remove old properties).
Merge pull request #13964 from code-dot-org/levelbuilder 
Merge pull request #13963 from code-dot-org/do-not-send-development-analytics Do not send analytics events in development or test
Merge pull request #13955 from code-dot-org/openid_connect_version fork omniauth-openid-connect
...

@Hamms
Copy link
Contributor

Hamms commented Mar 24, 2017

Woah, funky. I assume it has something to do with the way the PRs are being merged; presumably the GH web interface adds the title to the commit, but the DotD script does not

Looks simple enough to modify the merge_pull_request method call to add the title as a commit description

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.

None yet

2 participants