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

tools: ceph-release-notes: handle an edge case #16277

Merged
merged 1 commit into from Jul 13, 2017

Conversation

Projects
None yet
2 participants
@smithfarm
Contributor

smithfarm commented Jul 12, 2017

If the merge commit message consists of a single "Reviewed-by:" line, the
script fails at the line because len(lines) is zero.

duplicates_pr_title = lines[0] == pr['title'].strip()

Signed-off-by: Nathan Cutler ncutler@suse.com

tools: ceph-release-notes: handle an edge case
If the merge commit message consists of a single "Reviewed-by:" line, the
script fails at the line because len(lines) is zero.

    duplicates_pr_title = lines[0] == pr['title'].strip()

Signed-off-by: Nathan Cutler <ncutler@suse.com>
elif len(lines) == 1:
continue
assert len(lines) > 0, "missing message content"
if len(lines) == 1:

This comment has been minimized.

@tchaikov

tchaikov Jul 12, 2017

Contributor

shall we lines.pop(0) here? otherwise we would be looking at the first line. and it should be empty.

This comment has been minimized.

@smithfarm

smithfarm Jul 12, 2017

Contributor

@tchaikov It took me awhile to figure out what this code is actually doing. It's parsing the merge commit message (not the title). First, it removes any empty lines and lines that start with "Reviewed-by". Then, if there is only one (non-empty, non-Reviewed-by) line left, and it is different from the PR title, it replaces the PR title with that line. In all other cases, it appends the (non-empty, non-Reviewed-by) content from the merge commit message to the PR title.

Does my patch make more sense now, in light of the above?

This comment has been minimized.

@tchaikov

tchaikov Jul 13, 2017

Contributor

@smithfarm thanks for explaining this for me. the change makes lots of sense now!

@tchaikov tchaikov merged commit ffda75b into ceph:master Jul 13, 2017

3 of 4 checks passed

make check make check failed
Details
Signed-off-by all commits in this PR are signed
Details
Unmodified Submodules submodules for project are unmodified
Details
make check (arm64) make check succeeded
Details

smithfarm added a commit to smithfarm/ceph that referenced this pull request Jul 17, 2017

tools: ceph-release-notes: refactor and fix regression
This commit refactors the logic for determining the PR title and merge message,
and fixes a regression introduced by ceph#16277

Signed-off-by: Nathan Cutler <ncutler@suse.com>

smithfarm added a commit to smithfarm/ceph that referenced this pull request Jul 17, 2017

tools: ceph-release-notes: refactor and fix regression
This commit refactors the logic for determining the PR title and merge message,
and fixes a regression introduced by ceph#16277

Signed-off-by: Nathan Cutler <ncutler@suse.com>

smithfarm added a commit to smithfarm/ceph that referenced this pull request Jul 19, 2017

tools: ceph-release-notes: refactor and fix regression
This commit refactors the logic for determining the PR title and merge message,
and fixes a regression introduced by ceph#16277

Signed-off-by: Nathan Cutler <ncutler@suse.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment