Skip to content

Update PULL_REQUEST_TEMPLATE.md#983

Open
calellowitz wants to merge 1 commit intomainfrom
calellowitz-patch-2
Open

Update PULL_REQUEST_TEMPLATE.md#983
calellowitz wants to merge 1 commit intomainfrom
calellowitz-patch-2

Conversation

@calellowitz
Copy link
Copy Markdown
Collaborator

Technical Summary

Just updating the PR template to add a new field based on recent conversations about notifying delivery for database schema changes

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Feb 11, 2026

Walkthrough

This pull request adds a new "Schema Changes" section to the GitHub pull request template file. The section includes a checkbox item requiring contributors to confirm that breaking database schema changes have been communicated in the superset channel on Slack. The modification consists of 4 new lines added to the template with no removals.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~2 minutes

Suggested reviewers

  • sravfeyn
  • pxwxnvermx
  • Charl1996
  • hemant10yadav
🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 inconclusive)
Check name Status Explanation Resolution
Title check ❓ Inconclusive The title is vague and generic. It states only 'Update PULL_REQUEST_TEMPLATE.md' without conveying the specific change (adding a Schema Changes section for database change notifications). Revise the title to be more specific, such as 'Add Schema Changes section to PR template' or 'Add database schema change notification checkbox to PR template' to clearly convey the primary change.
✅ Passed checks (2 passed)
Check name Status Explanation
Description check ✅ Passed The description is related to the changeset. It mentions updating the PR template and adds context about database schema change notifications, which aligns with the actual change made.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch calellowitz-patch-2

Tip

Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In @.github/PULL_REQUEST_TEMPLATE.md:
- Line 36: Update the checklist item text that currently reads "[ ] Breaking
database schema changes have been communicated in the superset channel on slack"
to use the correct product capitalization: change "slack" to "Slack" so the line
reads "[ ] Breaking database schema changes have been communicated in the
superset channel on Slack".
🧹 Nitpick comments (1)
.github/PULL_REQUEST_TEMPLATE.md (1)

34-37: Consider clarifying the scope and channel reference.

A few suggestions to improve clarity:

  1. Scope: The checkbox currently only mentions "breaking" schema changes. Consider whether all database schema changes (breaking or non-breaking) should be communicated to the delivery team, as even non-breaking changes might affect reporting, analytics, or downstream systems.

  2. Channel format: Consider using the #superset format (with hash prefix) to make it clearer that this refers to a Slack channel name.

💡 Alternative wording options

Option 1 (broader scope):

 ### Schema Changes
 
-- [ ] Breaking database schema changes have been communicated in the superset channel on Slack
+- [ ] Database schema changes have been communicated in the `#superset` channel on Slack

Option 2 (keep breaking-only but clarify format):

 ### Schema Changes
 
-- [ ] Breaking database schema changes have been communicated in the superset channel on Slack
+- [ ] Breaking database schema changes have been communicated in the `#superset` channel on Slack


### Schema Changes

- [ ] Breaking database schema changes have been communicated in the superset channel on slack
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Capitalize "Slack" as it's a product name.

"slack" should be capitalized to "Slack" as it refers to the product/platform name.

📝 Proposed fix
-- [ ] Breaking database schema changes have been communicated in the superset channel on slack
+- [ ] Breaking database schema changes have been communicated in the superset channel on Slack
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
- [ ] Breaking database schema changes have been communicated in the superset channel on slack
- [ ] Breaking database schema changes have been communicated in the superset channel on Slack
🤖 Prompt for AI Agents
In @.github/PULL_REQUEST_TEMPLATE.md at line 36, Update the checklist item text
that currently reads "[ ] Breaking database schema changes have been
communicated in the superset channel on slack" to use the correct product
capitalization: change "slack" to "Slack" so the line reads "[ ] Breaking
database schema changes have been communicated in the superset channel on
Slack".

- Link to QA Ticket
-->

### Schema Changes
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I know I approved, but can we add a comment here explaining what "breaking changes" entails in the context of superset?

E.g.

<!--
- Any field renames
- Any field removals
- etc.
-->

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: Also, do we want to remove this section if there are no breaking changes? Similar to how on HQ we remove the section for migration reversal.
I am also okay with the current way this has been done. May be we can rephrase to say
"If there are breaking changes..."
the only confusing bit about that is that when someone reads they can't tell if there actually were breaking changes without reading the code changes.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

nit: Also, do we want to remove this section if there are no breaking changes? Similar to how on HQ we remove the section for migration reversal.

Can you say more? Is this a change we would make to the template in this PR, or just how you would use it?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Referring to this part in HQ template for PRs

This is simply to differentiate between

  1. there are no breaking changes
  2. breaking changes are present and have been communicated

Copy link
Copy Markdown
Contributor

@zandre-eng zandre-eng left a comment

Choose a reason for hiding this comment

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

+1 on Charl's suggestion for adding a comment.

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.

4 participants