raulcd commented on code in PR #33660: URL: https://github.com/apache/arrow/pull/33660#discussion_r1073389579
########## .github/pull_request_template.md: ########## @@ -57,5 +57,11 @@ If there are user-facing changes then we may require documentation to be updated --> <!-- -If there are any breaking changes to public APIs, please add the `breaking-change` label. ---> \ No newline at end of file +If there are any breaking changes to public APIs, please uncomment the line below and explain which changes are breaking. +--> +<!-- **This PR includes breaking changes to public APIs.** --> + +<!-- +Pleas uncomment the line below (and provide explanation) if the changes fix either (a) a security vulnerability, (b) a bug that caused incorrect or invalid data to be produced, or (c) a bug that causes a crash (even when the API contract is upheld). We use this to highlight fixes to issues that may affect users without their knowledge. For this reason, fixing bugs that cause errors don't count, since those are usually obvious. Review Comment: ```suggestion Please uncomment the line below (and provide explanation) if the changes fix either (a) a security vulnerability, (b) a bug that caused incorrect or invalid data to be produced, or (c) a bug that causes a crash (even when the API contract is upheld). We use this to highlight fixes to issues that may affect users without their knowledge. For this reason, fixing bugs that cause errors don't count, since those are usually obvious. ``` ########## docs/source/developers/reviewing.rst: ########## @@ -255,3 +255,43 @@ Social aspects * Like any communication, code reviews are governed by the Apache `Code of Conduct <https://www.apache.org/foundation/policies/conduct.html>`_. This applies to both reviewers and contributors. + + +Labelling +========= + +While reviewing PRs, we should try to identify whether these changes need to be +marked with one or both of the following labels: + +* **Critical Fix**: The change fixes either: (a) a security vulnerability; + (b) a bug that caused incorrect or invalid data to be produced; + or (c) a bug that causes a crash, though only if the API contract is upheld. + This is intended to mark fixes to issues that may affect users without their + knowledge. For this reason, fixing bugs that cause errors don't count, since + those bugs are usually obvious. Bugs that cause crashes are considered critical + because they are a possible vector of Denial-of-Service attacks. +* **Breaking Change**: The change breaks backwards compatibility in a public API. + For changes in C++, this does not include changes that simply break ABI + compatibility, except for the few places where we do guarantee ABI + compatibility (such as C Data Interface). Experimental APIs are *not* + exempt from this; they are just more likely to be associated with this tag. + +Breaking changes and critical fixes are separate: breaking changes alter the +API contract, while critical fixes make the implementation align with the +existing API contract. For example, fixing a bug that caused a Parquet reader +to skip rows containing the number 42 is a critical fix but not a breaking change, +since the row skipping wasn't behavior a reasonable user would rely on. + +These labels are used in the release to highlight changes that users ought to be +aware of when they consider upgrading library versions. Breaking changes help +identify reasons when users may wish to wait to upgrade until they have time to +adapt their code, while critical fixes highlight the risk in *not* upgrading. + +In addition, we use the following labels to indicate priority: + +* **Priority: Blocker**: Indicates the PR **must** be merged before the next Review Comment: At the moment for our release tasks we use the labels only on issues, not on PRs. Here we refer to labels on PRs and issues without distinction: `Indicates issues that are high priority.`. We also enforce the update of the milestone only on issues when merging a PR, not on PRs. Right now the commit title on the merge script allows us to link to the PR because we use the following: `commit_title = f'{self.title} (#{self.number})'` which links both the issue `GH-XXXX` and the PR `(#YYYYY)` even though at the moment we don't use the PR on our release tasks, we could. I suppose my concern is around the workflow changes and whether we want to force those labels only on issues, only on PRs, on both, what happens if a PR and an issue contain different types of labels, etcetera. This might affect the discussion about removing the merge script or at least how we link issues with PRs. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: github-unsubscr...@arrow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
