Skip to content

Support Release Notes Generation#13964

Closed
caithagoras wants to merge 4 commits intoprestodb:masterfrom
caithagoras:r1
Closed

Support Release Notes Generation#13964
caithagoras wants to merge 4 commits intoprestodb:masterfrom
caithagoras:r1

Conversation

@caithagoras
Copy link
Contributor

@caithagoras caithagoras commented Jan 14, 2020

The PR introduces a tool which formalize the release notes collection in the release process.
Address part of #13970.

Release Notes Collection

To streamline the release process, we introduced a requirement (and PR template) to ask authors to write release notes in the PR description. During the release process, the person doing the release will be able to use this tool to collect and generate the release commit and PR, before polishing it as according to the
Release Notes Guideline.

What does the tool do

  1. Switch to master branch. Fast-forward to upstream master.
  2. Use git log to find the commits introduced in the new release.
  3. Use Github GraphQL API v4 to fetch the PRs associated with the commits.
  4. Parse the PR description, generate release notes, create release branch, modify local files, create release notes commit, and push to origin.
  5. Create a release notes PR, with description populated. Release notes PR description contains 3 sections:
    a. A list of PRs with missing release notes, either the PR description do not contain release notes, or the release notes were malformed.
    b. A list of extracted release notes.
    c. A list of all commits introduced in the release.

How to use the tool

To collect release notes, run the following command after cutting the release branch (bumping the snapshot version on the master branch):

curl -L -o /tmp/presto_release "https://oss.sonatype.org/service/local/artifact/maven/redirect?g=com.facebook.presto&a=presto-release&v=0.232-20200123.012813-4&r=snapshots&c=executable&e=jar"
chmod 755 /tmp/presto_release
/tmp/presto_release release-notes --github-user <GITHUB_USER> --github-access-token <GITHUB_ACCESS_TOKEN>

The changes have been tested end-to-end. Idea for unit testing are welcomed.

== RELEASE NOTES ==
Release Tool Changes
* Add a tool to collect and generate release notes (:pr:`13964`).

@caithagoras caithagoras added the wip Work In Progress label Jan 14, 2020
@caithagoras caithagoras force-pushed the r1 branch 16 times, most recently from 1810871 to def7ec1 Compare January 15, 2020 22:34
@caithagoras caithagoras changed the title [WIP] Support Release Notes Generation Support Release Notes Generation Jan 15, 2020
@caithagoras caithagoras removed the wip Work In Progress label Jan 15, 2020
@caithagoras
Copy link
Contributor Author

@aweisberg Since @mayankgarg1990 is on PTO, could you help reviewing this PR? Thank you!

Copy link
Contributor

@aweisberg aweisberg left a comment

Choose a reason for hiding this comment

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

Wow, this is great, but also huge. GenerateReleaseNotesTask has a lot of behavior in it. It tested it and found a few things. I think I will want to do another pass on GenerateReleaseNotesTask.

Why an empty presto-release/README.txt?

Ultimately the documentation for this should live somewhere that isn't the PR description.

If you restart the process partway through it fails the 2nd time creating the branch. If you try to repeat the process it also fails. I assume this will be common because people fix the missing/broken release notes in their PRs and then you rerun this.

Copy link
Contributor

Choose a reason for hiding this comment

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

A little weird that it's a GitActor, but it's not a subclass of Actor. It makes them seem like they have something in common when apparently they don't.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is how objects are defined in Github API:
Actor (Interface): https://developer.github.com/v4/interface/actor/
GitActor (Object): https://developer.github.com/v4/object/gitactor/

My understanding is that the "Git" in "GitActor" refers to git, the version control system, while Actor is more of a short form for GithubActor.

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like some of this PR entry didn't get preserved #13604 All the lines starting with + got turned into a run-on sentence.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure what this comment means?

Copy link
Contributor

Choose a reason for hiding this comment

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

If you run this tool it generates
image
and
image
From that PR.

Copy link
Contributor Author

@caithagoras caithagoras Feb 14, 2020

Choose a reason for hiding this comment

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

This is the expected behavior since we never said we would support sub-bullet-point.
If the release notes section cannot be properly parsed, the PR will shown in the missing release notes section.

@caithagoras caithagoras force-pushed the r1 branch 4 times, most recently from 43fb4f9 to d2345d7 Compare January 17, 2020 23:56
@tdcmeehan
Copy link
Contributor

I came across this PR. I just skimmed and have a couple of high-level questions.

  1. Does this need to exist inside the main PrestoDB repo? Can it exist in a stand-alone repository? The Presto build is already long and the codebase already vast, it would be better to put this somewhere independent if there's no code sharing.
  2. The lines of code are quite hefty and we'll need to maintain this is as part of the mainline Presto build. Could it have been written more concisely with some 3rd party libraries, or even with a different language like Python?

Copy link
Contributor

@aweisberg aweisberg left a comment

Choose a reason for hiding this comment

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

This looks great. I had two minor things to tweak and there was the one case where it maybe doesn't handle the inputs the way we want.

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 most projects put the NOTICE file in a top level directory so it's human findable without much effort
https://github.com/docker/engine
https://github.com/apache/hadoop

How did we assemble the list of NOTICES we need to include?

Copy link
Contributor Author

@caithagoras caithagoras Feb 14, 2020

Choose a reason for hiding this comment

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

I'm moving NOTICE and README.md to the top level. They're specified in src/main/assembly/presto-release-tools.xml

Copy link
Contributor

Choose a reason for hiding this comment

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

Is what you are supposed to fetch going to change over time when the binary is updated? Is there a better way to do this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated. We can do v=LATEST to fetch the latest snapshot version. When there is a release of this project, we can change it to point to the latest release version.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you run this tool it generates
image
and
image
From that PR.

@caithagoras
Copy link
Contributor Author

Superseded by prestodb/presto-release-tools#2

@caithagoras caithagoras deleted the r1 branch February 17, 2020 21:25
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.

5 participants