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

chore: script for changelog #13588

Merged
merged 5 commits into from
Mar 17, 2021
Merged

Conversation

lilykuang
Copy link
Member

@lilykuang lilykuang commented Mar 12, 2021

SUMMARY

  • add a check for pull request with database migration
  • add --risk flag for getting a list pull request with labels blocking|risk|hold|revert|security vulnerability
  • print all change log with section of database migrations, features, fixes, and others

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

ADDITIONAL INFORMATION

  • Has associated issue:
  • Changes UI
  • Requires DB Migration.
  • Confirm DB Migration upgrade and downgrade tested.
  • Introduces new feature or API
  • Removes existing feature or API

@lilykuang lilykuang force-pushed the lily/change-log-script branch 4 times, most recently from 99ec6bf to 019aaf1 Compare March 12, 2021 23:52
@lilykuang lilykuang marked this pull request as ready for review March 12, 2021 23:52
@lilykuang lilykuang force-pushed the lily/change-log-script branch from 4d12d9d to b2b9364 Compare March 13, 2021 01:30
@lilykuang lilykuang force-pushed the lily/change-log-script branch from b2b9364 to 531828b Compare March 13, 2021 01:34
@lilykuang lilykuang force-pushed the lily/change-log-script branch from 531828b to 919165c Compare March 13, 2021 01:41
Copy link
Member

@betodealmeida betodealmeida left a comment

Choose a reason for hiding this comment

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

Looks great! I just left a few small comments and nits.

Comment on lines 125 to 128
for file in commit.files:
if "superset/migrations/versions/" in file.filename:
return True
return False
Copy link
Member

Choose a reason for hiding this comment

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

Nit suggestion:

Suggested change
for file in commit.files:
if "superset/migrations/versions/" in file.filename:
return True
return False
return any("superset/migrations/versions/" in file.filename for file in commit.files)

detail = self._pr_logs_with_details.get(pr_number)
if detail:
return detail
else:
Copy link
Member

Choose a reason for hiding this comment

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

Nit: no need for else here

Comment on lines 142 to 145
if pr_type:
pr_type = pr_type.group().strip('"')
else:
pr_type = None
Copy link
Member

Choose a reason for hiding this comment

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

pr_type will already by None if it's false, so:

Suggested change
if pr_type:
pr_type = pr_type.group().strip('"')
else:
pr_type = None
if pr_type:
pr_type = pr_type.group().strip('"')

Comment on lines 164 to 170
for label in labels:
risk_label = re.match(
r"^(blocking|risk|hold|revert|security vulnerability)", label.name
)
if risk_label is not None:
return True
return False
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we should move the regular expression r"^(blocking|risk|hold|revert|security vulnerability)" to a constant, then this could be written:

Suggested change
for label in labels:
risk_label = re.match(
r"^(blocking|risk|hold|revert|security vulnerability)", label.name
)
if risk_label is not None:
return True
return False
return any(re.match(RISKY_LABEL, label.name) for label in labels)

@@ -269,14 +368,20 @@ def compare(base_parameters: BaseParameters) -> None:
help="The github access token,"
" if not provided will try to fetch from GITHUB_TOKEN env var",
)
@click.option("--risk", is_flag=True, help="show all pull request with risky labels")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
@click.option("--risk", is_flag=True, help="show all pull request with risky labels")
@click.option("--risk", is_flag=True, help="show all pull requests with risky labels")

@lilykuang lilykuang merged commit 036fc39 into apache:master Mar 17, 2021
@lilykuang lilykuang deleted the lily/change-log-script branch March 17, 2021 15:55
betodealmeida pushed a commit to betodealmeida/incubator-superset that referenced this pull request Mar 19, 2021
* change log with section

* add risk flag for showing risk pull request

* update changlog.md

* lint mypy

* small fixes
@iercan
Copy link
Contributor

iercan commented Mar 26, 2021

@betodealmeida @lilykuang I couldn't see #13157 in changelog although it is tagged 1.1. Something wrong?

@lilykuang
Copy link
Member Author

@iercan thank you for checking the change log. #13157 is included in 1.1. The change log was updated before #13157 merged to 1.1 release. I opened an #13824 for correcting the 1.1 release change log.

cccs-rc pushed a commit to CybercentreCanada/superset that referenced this pull request Mar 6, 2024
* change log with section

* add risk flag for showing risk pull request

* update changlog.md

* lint mypy

* small fixes
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.2.0 labels Mar 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels preset-io size/L 🚢 1.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants