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

Edit release notes checklist in PR template #14018

Merged
merged 1 commit into from
Sep 19, 2022

Conversation

colebow
Copy link
Member

@colebow colebow commented Sep 6, 2022

Description

Update the PR template to use a GitHub task list in the release notes section, and also add an option for "this is a docs change and no release notes are required," because docs are user-visible but don't require release notes.

Non-technical explanation

Update PR template.

Release notes

(x) This is not user-visible and no release notes are required.
( ) This is a docs change and no release notes are required.
( ) Release notes are required, please propose a release note for me.
( ) Release notes are required, with the following suggested text:

@cla-bot cla-bot bot added the cla-signed label Sep 6, 2022
@colebow colebow requested review from findepi and mosabua September 6, 2022 17:30
@colebow
Copy link
Member Author

colebow commented Sep 6, 2022

Something worth pointing out:

image

We're going to see lots of 1/4 tasks complete on PRs, not sure if that's necessarily an issue, but it's at least a little annoying/suboptimal.

Copy link
Member

@mosabua mosabua left a comment

Choose a reason for hiding this comment

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

We should NOT use [] since the interferes with the GitHub actions reporting for build and CLA check and so on. We tried that initially with the template and it did not go well.

@colebow
Copy link
Member Author

colebow commented Sep 6, 2022

Are you sure? From the tests that @findepi and I have run, it doesn't seem like it's getting in the way?

@mosabua
Copy link
Member

mosabua commented Sep 6, 2022

Are you sure? From the tests that @findepi and I have run, it doesn't seem like it's getting in the way?

Hm .. interesting.. maybe they updated how this works. In either case .. since this list is a choice and not a task list I still think the reporting is misleading and we are better off not doing that. Is there any issue with the current setup and e.g. use (x) or (✅ ) or even just suggesting usage of ✅ and ❌ ?

@colebow
Copy link
Member Author

colebow commented Sep 6, 2022

I think I'm in agreement that task lists are not good, because GitHub integrates and visualizes them in a misleading way.

@colebow colebow force-pushed the colebow/pr-template-edit branch from 04f492b to 84c4923 Compare September 6, 2022 19:06
@colebow colebow requested a review from mosabua September 6, 2022 19:06
@colebow colebow force-pushed the colebow/pr-template-edit branch from fea74d6 to 84c4923 Compare September 6, 2022 19:17
@findepi
Copy link
Member

findepi commented Sep 6, 2022

@colebow i suggest checkboxes because

  • it allows easy edit in an intuitive way. Placing x within misaligned (non-monosapce font rendered) braces is not intuitive
  • it is very easy to read, as marked checkbox stands out

the interference with actions checkboxes or other aspects of PR status -- please do not worry about that. I'd let @trinodb/maintainers worry about that. It's their job to understand whether a thing is merge-ready.

IF we start to use checkboxes for things related to PR status THEN we may want to edit docs-related PR template, as it would THEN interfere with how we interpret the PR status / merge-readiness.

Copy link
Member

@findepi findepi left a comment

Choose a reason for hiding this comment

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

.

@colebow colebow force-pushed the colebow/pr-template-edit branch from 84c4923 to 312b4b3 Compare September 12, 2022 16:36
@colebow colebow requested a review from findepi September 12, 2022 16:36
@colebow colebow requested a review from findepi September 19, 2022 18:00
@colebow
Copy link
Member Author

colebow commented Sep 19, 2022

So as far as the checkboxes are concerned:

  • I think the upgrade they provide is minimal (instead of typing x, you click)
  • I think the downside they provide (surfacing an entirely new element of the GitHub UI, which is then misleading - see my first comment on this PR) is suboptimal and worth avoiding unless there is a very convincing reason

@findepi findepi merged commit ea9036f into trinodb:master Sep 19, 2022
@findepi
Copy link
Member

findepi commented Sep 19, 2022

whether "(instead of typing x, you click" is "minimal" or "surfacing an entirely new element of the GitHub UI" is important sounds subjective

To me, "instead of typing x, you click" is non-negligible thing. How are you making sure you're subjective truth is more right?

@github-actions github-actions bot added this to the 397 milestone Sep 19, 2022
@colebow colebow deleted the colebow/pr-template-edit branch October 27, 2022 04:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants