Site/contributing: add recommendations for working with PRs #1625
Conversation
snazy
requested review from
adutra,
ashvina,
dennishuo,
dimas-b and
eric-maynard
as code owners
snazy
requested review from
HonahX,
MonkeyCanCode,
RussellSpitzer,
collado-mike,
ebyhr,
flyrain,
jackye1995,
jbonofre,
singhpk234,
takidau and
vvcephei
as code owners
CONTRIBUTING.md
Outdated
There was a problem hiding this comment.
Is this a best practice? When reviewing, I really dislike when people have done this because it makes it harder to see the changes since I last reviewed, or even the difference between two states of the change.
There was a problem hiding this comment.
This is about "Opening" a PR, no?
There was a problem hiding this comment.
My question stands, not sure how critical this step is. Smaller incremental commits is generally a good thing IMO.
There was a problem hiding this comment.
@eric-maynard this is to get to better messages. If you do it in another way but provide good PR + Git-merge-commit summary/description that's fine, too.
Also I don't understand your argument about reviews - again - this paragraph is about opening the PR - there's nothing else you could review before, right?
There was a problem hiding this comment.
If this is to get better messages, then I'll just point to the other comment chain about using the PR description.
If this is about reviewability, I maintain that more commits is usually a good thing.
There was a problem hiding this comment.
Would it make sense to update the sentence from
"Before opening a pull request, squash all commits in your branch into a single commit"
to
"Before opening a pull request, squash commits in your branch into a few (or single) meaningful commits"
There was a problem hiding this comment.
I'm also not sure why we would want to require a single commit. I'd probably just drop this line and just require that folks add a meaningful summary in the PR summary.
There was a problem hiding this comment.
It's meant as a "tip" - moved the controversial one to a separated "Tips" section.
|
Question here: should we be doing Squash-and-Merges when merging code in? I'm assuming that would help with some of these issues like the changelogs - but in that case, the PR title and description still needs to be set correctly by the PR author. |
We've decided very early on that we want to have no merge-commits. |
eric-maynard
left a comment
eric-maynard
left a comment
There was a problem hiding this comment.
Most changes LGTM, but some of the ideas here were not discussed in the mailing list and it seems that we may be able to reconfigure the repo to loosen some of the requirements around commit messages
The justification is missing concrete pointers. Also requesting another change pushed into this change, which is clearly out of scope of this PR. |
CONTRIBUTING.md
Outdated
There was a problem hiding this comment.
Do we need this rule? I'm not sure we discussed it on the thread and I'd like to minimize the number of rules a newcomer to the project has to follow.
There was a problem hiding this comment.
It's also rather a tip. We had a few occurrences where PRs were in "ready for review" but were actually "work in progress" or "superseded" or "paused". Moved this to a separate "Tips" section.
This change updates the PR guidelines on the "Contributing" web page after [this discussion](https://lists.apache.org/thread/kfxo3cqmw3pgrpgtgqvqpwvn46yw8q7h).
snazy
force-pushed
the
contributing-commit-msg
branch
from
7a610bd to
0c28152
Compare
|
Hi @eric-maynard, the controversial sections in this PR have been addressed. Can you please take a look at them, review the changes, and reassess your -1 |
github-project-automation
bot
moved this from PRs In Progress
to Ready to merge
in Basic Kanban Board
github-project-automation
bot
moved this from Ready to merge
to Done
in Basic Kanban Board
|
Thx! Merged. |
* main: Update dependency org.postgresql:postgresql to v42.7.6 (apache#1697) * main: Update helm/chart-testing-action action to v2.7.0 (apache#1700) * main: Update gradle/actions digest to 8379f6a (apache#1696) * main: Update medyagh/setup-minikube action to v0.0.19 (apache#1698) * feat(metrics): Mitigate potential performance issues with realm_id tag (apache#1662) As discussed in the ML, this PR introduces two flags to enable the inclusion of realm ID tags in API and HTTP metrics. They are both disabled by default. There is also a new safeguard: if the cardinality of realm IDs in HTTP metrics goes above a configurable threshold (100 by default), a warning is printed and no more HTTP metrics will be recorded. (Quarkus has a similar safeguard for URI tags in HTTP metrics.) * Site/contributing: add recommendations for working with PRs (apache#1625) This change updates the PR guidelines on the "Contributing" web page after [this discussion](https://lists.apache.org/thread/kfxo3cqmw3pgrpgtgqvqpwvn46yw8q7h). Also adopt `gradlew test` to `gradlew check` in README, following the intent (all tests, incl ITs) * main: Update dependency com.azure:azure-sdk-bom to v1.2.35 (apache#1703) * main: Update dependency com.adobe.testing:s3mock-testcontainers to v4.4.0 (apache#1705) * main: Update dependency boto3 to v1.38.24 (apache#1702) * Keep generated RSA-key-pair for JWT token broker on heap (apache#1661) Polaris allows using RSA key-paris for the JWT token broker. The recommended way is to [generate the RSA key pair](https://github.com/apache/polaris/blob/d8b862b13914d526ee147dc0e359bfc9c1e319ad/site/content/in-dev/unreleased/configuring-polaris-for-production.md?plain=1#L61-L66) and configure the location of the key files. However, if only `polaris.authentication.token-broker.type=rsa-key-pair` but not the `public/private-key-pair` options are configured, Polaris generates those and stores them in `/tmp` using random file names (using `Files.createTempFile()`) - this happens for each (matching) realm. Each Polaris startup generates new key-pairs for each of those realms. It's practically not possible to associate the files to a realm. There is already a [production readiness check](https://github.com/apache/polaris/blob/d8b862b13914d526ee147dc0e359bfc9c1e319ad/quarkus/service/src/main/java/org/apache/polaris/service/quarkus/config/ProductionReadinessChecks.java#L118-L166) to warn users about this behavior. Due to the issue that the files cannot be associated, those seem to be somewhat useless and bring no advantage over keeping these "ephemeral RSA key pairs" on heap. This PR changes the code to not write the key-pair to the file system and keeps these "ephemeral key pairs" on heap. Since the same code path is used for key-paris _provided_ by the user (via the `public/private-key-pair` config options), that code path now only reads those files once and not every time the private/public key is needed. * Merge JPA module with EclipseLink Module (apache#1718) * Create LICENSE and NOTICE for "single" distribution (apache#1694) * Fix a failing task with the release profile (apache#1693) * Remove unused adminDocs artifact (apache#1749) * Production readiness for Persistence (apache#1707) Production readiness for Persistence (apache#1707) * Fixes for direct usage of client_secret apache#1756 When the spec was upgraded and the python client regenerated from it, clientSecret was made a password, which means calling str on it directly yields a redacted string like ******. In the initial PR to change the python client and fix regtests, some existing usage of client_secret was not changed. * main: Update dependency org.junit:junit-bom to v5.13.0 (apache#1760) * main: Update dependency org.testcontainers:testcontainers-bom to v1.21.1 (apache#1748) * fix: Improve reliability of metrics tests (apache#1763) CI sometimes fails with errors like "http_server_requests_seconds not found" in the reported metrics. This looks like a race between the Quarkus metrics producer and the tests asking for these metrics. This change adds a time-limited retry loop until the expected metrics are available, before proceeding with other assertions. Note: in normal cases the loop finishes fast because the metrics are available. The two-minute timeout would apply only when the expected metrics fail to be produced at all. * Fix test_spark_credentials_s3_exception_on_metadata_file_deletion (apache#1759) * Regenerate bundled spec & Regenerate Python client (apache#1751) I ran these commands from main: ``` redocly bundle spec/polaris-catalog-service.yaml -o spec/generated/bundled-polaris-catalog-service.yaml ./gradlew regeneratePythonClient ``` I didn't realize before that some Python types are generated form the bundled spec, so some of the fixes from apache#1347 didn't get properly applied before. * main: Update dependency boto3 to v1.38.27 (apache#1714) * NoSQL: bump Weld/Junit5 (fixes a bug that surfaces w/ JUnit 5.13) * NoSQL: Let some more tests leverage Jandex * Info: Last merged commit b7aac72 --------- Co-authored-by: Mend Renovate <bot@renovateapp.com> Co-authored-by: Alexandre Dutra <adutra@users.noreply.github.com> Co-authored-by: Yufei Gu <yufei@apache.org> Co-authored-by: JB Onofré <jbonofre@apache.org> Co-authored-by: Prashant Singh <35593236+singhpk234@users.noreply.github.com> Co-authored-by: Eric Maynard <eric.maynard+oss@snowflake.com> Co-authored-by: Dmitri Bourlatchkov <dmitri.bourlatchkov@gmail.com> Co-authored-by: gh-yzou <167037035+gh-yzou@users.noreply.github.com>
5 participants
This change updates the PR guidelines on the "Contributing" web page after this discussion.