Change GH api used to build release notes; plus refactoring #11645
Conversation
|
Jenkins results:
|
There was a problem hiding this comment.
@amaltaro There are few things I have noticed in this PR. Please take a look before merging. Other than that, it looks good and the results it produces are quite sensible and seem to be solving the issue.
bin/buildrelease.sh
Outdated
|
|
||
| # Grab all the commit hashes, subject and author since the last release | ||
| TMP_HASHES_SUBJ_AUTHOR=$(mktemp -t wmcore_hashes.${LASTVERSION}.XXXXX) | ||
| git log --no-merges --pretty=format:'%H %s (%an)' ${LASTCOMMIT}.. >> $TMP_HASHES_SUBJ_AUTHOR |
There was a problem hiding this comment.
Alan, this line does not generate anything until it has the two dots at the end (..), while removing this results in the following output:
$ git log -3 --no-merges --pretty=format:'%H %s (%an)' ${LASTCOMMIT}
55d76e3022bd498258767f9c187b4fa02d7a17af 2.2.2rc5 (Alan Malta Rodrigues)
0524a82b734d351b998829ad555704300da78b13 2.2.2rc4 (Alan Malta Rodrigues)
a5a824ab603c060600f7ed3215618f2339560911 Add Docker context path to docker/build-push-action@v1 (Todor Ivanov)
There was a problem hiding this comment.
Yes, that is correct. But is it just an observation or are you actually requesting any changes here?
There was a problem hiding this comment.
Sorry Alan, my bad, I was not considering the fact that $LASTCOMMIT is actually the limiting commit freventing from going through the whole repository history. You may ignore my comment bellow as well.
bin/buildrelease.sh
Outdated
| PR=$(curl -s https://api.github.com/repos/dmwm/WMCore/commits/$HASH_ID/pulls | grep -Po '\"html_url\": \"https://github.com/dmwm/WMCore/pull/\K[0-9]+' | sort | uniq) | ||
| # remove hash_id from the commit line | ||
| commitline=$(echo $commitline | sed 's+$HASH_ID+ -+') | ||
| echo "$commitline #$PR" >> TMP_CHANGES |
There was a problem hiding this comment.
Missing $ sign: $TMP_CHANGES
| PR=$(curl -s https://api.github.com/repos/dmwm/WMCore/commits/$HASH_ID/pulls | grep -Po '\"html_url\": \"https://github.com/dmwm/WMCore/pull/\K[0-9]+' | sort | uniq) | ||
| # remove hash_id from the commit line | ||
| commitline=$(echo $commitline | sed 's+$HASH_ID+ -+') | ||
| echo "$commitline #$PR" >> TMP_CHANGES | ||
| done |
There was a problem hiding this comment.
This cycle goes to beginning of time - iterates through the whole repository history and does not stop at some specific commit. I am not sure if this is the expected behavior, but it takes quite a lot of time.
There was a problem hiding this comment.
The use case is: given a recent list of commit hash ids, fetch their equivalent pull request numbers. Building the release notes takes around a second, so I it's not clear to me what issue you are reporting here.
There was a problem hiding this comment.
not a problem here... see my comment above
|
Jenkins results:
|
|
@todor-ivanov I fixed one missing dollar sign that you noted - I think it was in the middle of your review - so you might want to have another look. |
revert editor add missing dollar sign sed with double quotes instead of single Replace format option from n to N for the author
|
Now that we have more commits at the HEAD, I could execute the final tests. I had to update 2 lines in the previous code:
Documentation on these log format can be found at: https://devhints.io/git-log-format |
|
Jenkins results:
|
|
Jenkins results:
|
|
This is working fine now. Merging |
amaltaro
changed the title
Fixes #11640
Status
ready
Description
Some overall refactoring of this script, it's worth highlighting though:
Is it backward compatible (if not, which system it affects?)
YES
Related PRs
None
External dependencies / deployment changes
None