add dump-issues subparser by aabadie · Pull Request #9 · RIOT-OS/RIOT-release-manager

aabadie · 2019-01-31T09:30:25Z

Adapted the riot-release-manager script with a dump-issues subparser. It dumps all known issues (e.g. with label Type: Bug) classified by main types (network, timers, drivers, etc) and also all closed issues since latest release.

The output can be copy-pasted in the release notes file of RIOT. I'm using this to generated the issues list in RIOT-OS/RIOT#10871

Later potential improvements:

Modify the script to automatically open a PR with release notes
Use Jinja2 template engine to generate the release notes
Add more information: line modified, PR merged, etc
etc

Usage

Just call riot-release-manager dump-issues

miri64 · 2019-02-01T16:26:13Z

Shouldn't the bugs in the release notes not just be "highlighted" bugs?

aabadie · 2019-02-01T17:34:56Z

The script is a bit systematic indeed, since all issues labelled with Type: Bug are taken into account. But that helps a lot for generating this list.

I have no idea which bug should be highlighted and others not. At least, here we are transparent about the issues we have. Maybe a bit of cleanup could be done between all of them?

MrKevinWeiss · 2019-06-12T07:53:39Z

I think this really helps, I was able to use it pretty easy. Even if some manual work is needed it still helps. Should we try to get it in?

miri64 · 2019-06-12T09:13:21Z

Even if some manual work is needed it still helps. Should we try to get it in?

Maybe some keyword that is added by this subroutine and then checked by the final release subroutine could be added, so lazy release managers don't accidentally release the issue list unchecked could be beneficial?

aabadie · 2019-09-21T10:02:22Z

I think this PR has proved that it was useful. Do we agree to merge it ? (I can squash if someone agrees).

miri64

I leave the testing to @kb2ma. However some minor issues I picked up reading the code.

aabadie · 2019-09-24T18:47:42Z

Sorry, I messed with my rebase, so I squash all commits into one. Hope this is fine. Tested again locally and still working as before.

miri64 · 2019-09-24T18:48:11Z

I'm fine with the PR is it is @kb2ma I leave it to you to test :-).

kb2ma

Thanks for this PR, @aabadie! I ran it once, and it basically looks OK. I will look more closely at the content to be sure.

It definitely needs an addition to README.rst like the other commands, to describe what it is and how to use it.

aabadie · 2019-10-28T14:46:21Z

@kb2ma, I added an entry in the README for dump-issues and fixed the typo you reported in the function.

Comments addressed.

kb2ma

I like how the code builds known_issues to ultimately print the list of uncategorized issues. A couple of comments to make this explicit will make it easier for a future reader to quickly understand how it works.

kb2ma · 2019-10-30T13:02:08Z

+
+
+def print_open_issues(repo, msg, labels=None, known_issues=None):
+    """Retrieve and print all open issues."""


Please add to this comment that the input list of known_issues is updated and returned.

kb2ma · 2019-10-30T13:06:02Z

+    minor_label = repo.get_label('Impact: minor')
+    excluded_issues = get_issues(repo, 'open', labels=[minor_label])
+
+    # Network related issues


Please add a short summary of how this works. The list of known_issues is incrementally updated with each call to print_open_issues until finally only open issues not yet printed are included with "other issues".

kb2ma · 2019-10-30T13:22:40Z

I have looked in detail at the code now, and asked for a couple of changes. It all works AFAICT, which is the most important thing. :-) Aside from that, see the suggestions below for follow-up work.

I definitely would move the issue code from this PR to a separate module (python file). It logically covers a separate topic from the rest of release_manager.py. Also, the technique of building known_issues works fine, but the operation of the code would be clearer and simpler if there was a module level variable to hold the list of known issues, rather than having to pass them through print_open_issues() each time.

Print the number of issues found on a separate line at the end of print_issues(). This will allow us to easily see how the number of issues changes from release to release.

Right justify the issue numbers so they all line up. Some have 5 digits and some have 4.

aabadie · 2019-11-03T16:57:29Z

I definitely would move the issue code from this PR to a separate module (python file). It logically covers a separate topic from the rest of release_manager.py

I disagree with this. This script is here to help the release manager when using git and github: it creates the release specs issues, creates the tags/branches, publish the release. To do this, it requires access to the github API (with API tokens, url, everything). Extending the existing module is perfectly fine IMHO. This is what this PR is doing.

the technique of building known_issues works fine, but the operation of the code would be clearer and simpler if there was a module level variable to hold the list of known issues, rather than having to pass them through print_open_issues() each time.

I pushed a fixup commit that changes this like you suggest.

Print the number of issues found on a separate line at the end of print_issues(). This will allow us to easily see how the number of issues changes from release to release.

Done. Also added the number of open issues per section and the number of closed issues since last release.

Right justify the issue numbers so they all line up. Some have 5 digits and some have 4.

Doable but note that each line is printed as it comes out from the GitHub API. That would need some extra processing to know how to justify the issue number. Is this required ?

miri64 · 2019-11-03T23:03:43Z

+    for issue in gh_issues:
+        if issue.pull_request is not None:
+            continue
+        msg = u"#{}: {}".format(issue.number, issue.title)


Right justify the issue numbers so they all line up. Some have 5 digits and some have 4.

Doable but note that each line is printed as it comes out from the GitHub API. That would need some extra processing to know how to justify the issue number. Is this required ?

Here you reformat it yourself, so you can just pre-format the number and then use alignment...

Suggested change

msg = u"#{}: {}".format(issue.number, issue.title)

msg = u"{:6s}: {}".format("#{}".format(issue.number), issue.title)

Thanks! Indeed, it was in the code. Wrote this so long ago...

miri64 · 2019-11-03T23:08:41Z

I disagree with this. This script is here to help the release manager when using git and github: it creates the release specs issues, creates the tags/branches, publish the release. To do this, it requires access to the github API (with API tokens, url, everything). Extending the existing module is perfectly fine IMHO. This is what this PR is doing.

I agree with @kb2ma here. a) The issue dumper is not as automated as the others (otherwise you could just create a PR from the generated stuff), b) I don't get the argument about the API... Putting the stuff in its separate .py file doesn't block you from using the GitHub API.

aabadie · 2019-11-04T06:50:19Z

a) The issue dumper is not as automated as the others (otherwise you could just create a PR from the generated stuff), b) I don't get the argument about the API

If you think a little further, the whole changelog generation could be automatized. The dump-issues is just a small step toward this. Once done, adding the changelog and creating the PR, etc could be automatized like other stuff.
I think this was what I had in mind initially, but as usual it takes so much time to get reviews and be merged in general with the RIOT community that in the end, you almost forget your initial goal...

Putting the stuff in its separate .py file doesn't block you from using the GitHub API.

Sure, but that is enforcing me to also put all github info in a separate module, update the existing module (maybe split this one in more pieces) and create a new module for dump-issues parser. It's not that much but this is extra work and I'm not convinced this is useful now.

kb2ma · 2019-11-04T13:56:57Z

I think this was what I had in mind initially, but as usual it takes so much time to get reviews and be merged in general with the RIOT community that in the end, you almost forget your initial goal...

I am sensitive to this myself. I hope my original comment was not confusing. For this PR I was just looking for addition of a couple of comments about how print_open_issues() works, and the rest was "suggestions for follow-up work", which meant for a follow-up PR.

At any rate, I think the change to use the module level KNOWN_ISSUES is a significant improvement because it removes all of the passing around of this collection. I definitely think the structure is good enough for this PR. It's also nice to have the count of issues.

I did notice one surprise. Did you intend to repeat the count of fixed issues at the end, since it already is included in the header?

...
There are 121 known issues in this release

Fixed Issues from the last release (2019.10) (1)
================================================

#11908: cpu/lpc2387: periph_rtc completely broken

1 closed issues since last release (2019.10)

aabadie · 2019-11-04T14:26:09Z

Did you intend to repeat the count of fixed issues at the end, since it already is included in the header?

No this is an undesirable side effect unfortunately. I'd like to remove it from the header (to be consistent with known issues header).

kb2ma · 2019-11-07T10:47:41Z

@aabadie, it's been a couple of days, do you plan to make this change to the current PR? If not, I am happy to just approve this because the extra count display is just cosmetic.

Issues addressed

aabadie · 2019-11-07T13:43:19Z

do you plan to make this change to the current PR?

Thanks for the reminder. This is fixed, squashed and pushed.

kb2ma · 2019-11-07T18:00:49Z

Good stuff! The latest update works and code makes sense. I leave it to you for any documentation on future improvements.

aabadie · 2019-11-07T18:47:32Z

Thank you for the review @kb2ma and @miri64!

aabadie requested a review from miri64 January 31, 2019 09:30

miri64 mentioned this pull request Sep 18, 2019

stale: don't stale issues labelled with "Type: bug" RIOT-OS/RIOT#12192

Merged

miri64 reviewed Sep 23, 2019

View reviewed changes

Comment thread riot_release_manager.py Outdated

Comment thread riot_release_manager.py Outdated

aabadie force-pushed the dump_issues branch 3 times, most recently from b12f81d to d60ba9a Compare September 24, 2019 18:44

kb2ma self-requested a review October 25, 2019 12:28

kb2ma previously requested changes Oct 25, 2019

View reviewed changes

kb2ma reviewed Oct 28, 2019

View reviewed changes

Comment thread riot_release_manager.py Outdated

aabadie force-pushed the dump_issues branch from d60ba9a to 8db4c6f Compare October 28, 2019 14:39

kb2ma previously requested changes Oct 30, 2019

View reviewed changes

miri64 reviewed Nov 3, 2019

View reviewed changes

add dump-issues subparser

16d2d9f

README: add entry for dump-issues subparser

b71c337

aabadie force-pushed the dump_issues branch from e7d5f79 to b71c337 Compare November 7, 2019 13:42

kb2ma merged commit 214d8d6 into master Nov 7, 2019

miri64 deleted the dump_issues branch November 7, 2019 18:31



		def print_open_issues(repo, msg, labels=None, known_issues=None):
		"""Retrieve and print all open issues."""

	msg = u"#{}: {}".format(issue.number, issue.title)
	msg = u"{:6s}: {}".format("#{}".format(issue.number), issue.title)

Uh oh!

Conversation

aabadie commented Jan 31, 2019

Usage

Uh oh!

miri64 commented Feb 1, 2019

Uh oh!

aabadie commented Feb 1, 2019

Uh oh!

MrKevinWeiss commented Jun 12, 2019

Uh oh!

miri64 commented Jun 12, 2019

Uh oh!

aabadie commented Sep 21, 2019

Uh oh!

miri64 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

aabadie commented Sep 24, 2019

Uh oh!

miri64 commented Sep 24, 2019

Uh oh!

kb2ma left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aabadie commented Oct 28, 2019

Uh oh!

kb2ma left a comment

Choose a reason for hiding this comment

Uh oh!

kb2ma Oct 30, 2019

Choose a reason for hiding this comment

Uh oh!

kb2ma Oct 30, 2019

Choose a reason for hiding this comment

Uh oh!

kb2ma commented Oct 30, 2019

Uh oh!

aabadie commented Nov 3, 2019

Uh oh!

miri64 Nov 3, 2019

Choose a reason for hiding this comment

Uh oh!

aabadie Nov 4, 2019

Choose a reason for hiding this comment

Uh oh!

miri64 commented Nov 3, 2019

Uh oh!

aabadie commented Nov 4, 2019

Uh oh!

kb2ma commented Nov 4, 2019

Uh oh!

aabadie commented Nov 4, 2019

Uh oh!

kb2ma commented Nov 7, 2019

Uh oh!

aabadie commented Nov 7, 2019

Uh oh!

kb2ma commented Nov 7, 2019

Uh oh!

aabadie commented Nov 7, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants