Skip to content

[MINOR][INFRA] Add a guide to clarify release/unreleased Spark versions of user-facing change in the Github PR template#28403

Closed
HyukjinKwon wants to merge 1 commit intoapache:masterfrom
HyukjinKwon:minor-more-guide
Closed

[MINOR][INFRA] Add a guide to clarify release/unreleased Spark versions of user-facing change in the Github PR template#28403
HyukjinKwon wants to merge 1 commit intoapache:masterfrom
HyukjinKwon:minor-more-guide

Conversation

@HyukjinKwon
Copy link
Member

What changes were proposed in this pull request?

This PR proposes to add a guide to clarify the Spark version when describing "Does this PR introduce any user-facing change?".

Why are the changes needed?

It seems confusing to write when the user facing changes happen within unreleased branches.

Does this PR introduce any user-facing change?

No, dev-only.

How was this patch tested?

Manually tested in Github and it renders find as intended.

@HeartSaVioR
Copy link
Contributor

HeartSaVioR commented Apr 29, 2020

It sounds like non-minor thing which worths to initiate discussion.

From my experience, first two sections have been ended up describing the similar things, hence many times I just mentioned that previous/next section covers it.

I'm also wondering about the benefits of being strict about the section Does this PR introduce any user-facing change?. This seems to be adopted from Kubernetes, but after I looked into Kubernetes template, the meaning of the section looks to be very different.

https://raw.githubusercontent.com/kubernetes/kubernetes/master/.github/PULL_REQUEST_TEMPLATE.md

**Special notes for your reviewer**:

**Does this PR introduce a user-facing change?**:
<!--
If no, just write "NONE" in the release-note block below.
If yes, a release note is required:
Enter your extended release note in the block below. If the PR requires additional action from users switching to the new release, include the string "action required".

For more information on release notes see: https://git.k8s.io/community/contributors/guide/release-notes.md
-->
\```release-note

\```

**Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.**:

<!--
This section can be blank if this pull request does not require a release note.

When adding links which point to resources within git repositories, like
KEPs or supporting documentation, please reference a specific commit and avoid
linking directly to the master branch. This ensures that links reference a
specific point in time, rather than a document that may change over time.

See here for guidance on getting permanent links to files: https://help.github.com/en/articles/getting-permanent-links-to-files

Please use the following format for linking documentation:
- [KEP]: <link>
- [Usage]: <link>
- [Other doc]: <link>
-->
\```docs

\```

It would be mostly NONE, and require description only when the change worths to put into release note. I personally find the section useful to describe breaking backward compatibility, guide reviewers to focus more on the change, but if we need to put the information for any changes being shown to users (even doc) then it would become the huge pain.

Have we gone through the PR template or contribution guide doc for similar size of open source projects?

@HeartSaVioR
Copy link
Contributor

HeartSaVioR commented Apr 29, 2020

Oh there's another doc describing when to write release notes.
https://github.com/kubernetes/community/blob/master/contributors/guide/release-notes.md

At least it seems to be broader than I imagine, but still not sure the benefits of be strict about it.

Shall we please have some reference PRs (best practice in k8s repo), and see what's missing here?

@SparkQA
Copy link

SparkQA commented Apr 29, 2020

Test build #122053 has finished for PR 28403 at commit 540c600.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@HyukjinKwon
Copy link
Member Author

HyukjinKwon commented Apr 29, 2020

The benefit from this is to make it easier to track what caused a specific regression or behaviour change. Spark is being mature, and now it's getting more and more difficult to track the changes in the history.

Also, note that this is rather a guidance than a strict requirement. I believe this doesn't block a PR. Well, this will likely be asked by a committer though. It actually makes the review/merge process faster. I myself happen to duplicate some contents time to time but I still see this is missing in other PR descriptions.

I don't think the meaning vs k8s template is very different. We're just more conservative on the user-facing change - think about the Micheal's rubric of the amended semver as well.

More importantly, what this PR proposes is just to guide contributors to know what to write there more explicitly and remove the ambiguity there. Changing the contents is orthogonal.

@HyukjinKwon
Copy link
Member Author

retest this please

@SparkQA
Copy link

SparkQA commented Apr 29, 2020

Test build #122069 has finished for PR 28403 at commit 540c600.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@HyukjinKwon
Copy link
Member Author

Thanks, merged to master.

@HyukjinKwon HyukjinKwon deleted the minor-more-guide branch July 27, 2020 07:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

Comments