linter: Add PR check to encourage including issue or epic refs in the PR/commit messages #77376

jlinder · 2022-03-04T03:38:45Z

Describe the solution you'd like

This new linter should be a program run by a GitHub action on every PR for these events: create, edit, reopen, synchronize.

What it will do:

For all PRs containing commits with Release note texts, other than bug fixes, Engineers can help establish the link to Jira epics in various ways:

If the PR relates to only one Jira epic:
- Put the GH issue or Jira epic reference either in the PR description or in each relevant commit.
- If the reference is in the PR description, put “Epic: None” in each commit that doesn’t relate to the epic. This is particularly important if the commit contains a release note.
If the PR relates to multiple Jira epics:
- Put the GH issue or Jira epic reference in each relevant commit.
- If a single commit relates to multiple Jira epics, put all relevant GH issue or Jira epic references in that commit.
- If all commits relate to all the Jira epics, put the GH issue or Jira epic references in the PR description or in the commit messages.
- Put “Epic: None” in each commit with a release note that doesn’t relate to any epic.
If the PR relates to no Jira epics:
- Preferably, put “Epic: None” in the PR description or in each commit containing a release note.

Given Engineering’s practice to link GitHub issues to epics, referencing such an issue—as opposed to the epic it links to—is preferred in cases where both a Jira epic and GitHub issue exist. This makes information in the issue easily accessible to human readers while offering an epic link that is available via the issue to automation scripts.

The “Epic: None” option accounts for the fact that a significant amount of product changes occur spontaneously (serendipitously) as a by-product of investigations on main roadmap items. When this happens, Engineers take the initiative to make user-facing changes without prior processing by a PM, and thus without prior existence of a GH or Jira issue. We do not wish to reduce the ability of Engineers to innovate and improve our product in this way.

This solution will be encouraged, but not required, by a new optional blathers check on PRs. It will fail and give feedback on how to adjust the PR for the following cases:

When no GH issue reference, Jira epic reference, or “Epic: None” exists in the PR description or commit messages.
When multiple Jira epic references are in the PR description but not in individual commit messages.
When multiple Jira epic references are in a commit message (give a warning that it’s not a common case and to check that they intend it).
When backport PRs backport multiple source PRs and one or more of the source PRs has one of the above cases.

Describe alternatives you've considered

This linter can't run in TeamCity because it needs to run on PR edit and reopen events. We considered using Blathers but decided to keep this in the cockroach repo.

Jira issue: CRDB-13558

Epic DEVINF-261

99368: workload/schemachanger: basic declarative schema changer support r=fqazi a=fqazi Previously, the randomized testing only supported, the non-declarative schema changer. This patch adds support for basic single statement transactions for testing the declarative schema changer Note: This entablement is only focused on very minimal stability testing. Not implemented errors are always ignore for now. Fixes: #77376 Release note: None 100313: bazel: update toolchains to support remote execution r=rail a=rickystewart We need to add some extra files to `compiler_files` and `linker_files` and update some compiler flags to make sure remote execution works. Epic: none Release note: None 100596: testserver: fix version gate for tenant capabilities r=rafiss a=rafiss This was revealed by mixed version testing, and it can flake sometimes. Epic: None Release note: None Co-authored-by: Faizan Qazi <faizan@cockroachlabs.com> Co-authored-by: Ricky Stewart <rickybstewart@gmail.com> Co-authored-by: Rafi Shamim <rafi@cockroachlabs.com>

Previously, the combining of constraint spans when one constraint is a suffix of the other could take a long time and cause the `statement_timeout` session setting to not be honored when each constraint has hundreds or thousands of spans. The issue is that `constraint.Combine` has double nested loops to consider every combination one span from one constraint with one span of the other constraint, and if the system is under memory pressue, the large number of memory allocations made to perform these combinations and generate new spans may stall attempting to make those allocations. The fix is to maintain a counter in `constraint.Combine` and call the query cancel check function every 16 iterations. The cancel check function itself will check for query timeout every 1024 iterations, so effectively every 16K iterations `constraint.Combine` will perform cancel checking and abort the query if the timeout has been reached. Epic: none Fixes: cockroachdb#77376 Release note (bug fix): This patch fixes an issue where the optimizer fails to honor the `statement_timeout` session setting when generating constrained index scans for queries with large IN lists or `= ANY` predicates on multiple index key columns. The problem may be more noticeable when the system is under memory pressure.

In [1], the default of `UseSpotVM` was reinterpreted to mean the framework decides whether or not to create a spot VM. This may impede local (performance) testing when a potential preemption is unwelcome; i.e., may result in loss of work. This change reverts the original default of "no SpotVMs". It now interprets `--use-spot [true]` to mean that the framework decides whether or not to create a spot VM, whereas `--use-spot false` (default) unconditionally disables the use of spot VMs. Furthermore, to allow unconditional creation of spot VMs, the flag `--use-spot-always` is added. Finally, when running in CI, we pass `--use-spot true`, at this time, to allow the framework to conditionally create spot VMs, i.e., based on current heuristics. [1] #116778 Epic: none Fixes: #77376 Release note: None

In [1], the default of `UseSpotVM` was reinterpreted to mean the framework decides whether or not to create a spot VM. This may impede local (performance) testing when a potential preemption is unwelcome; i.e., may result in loss of work. This change reverts the original default of "no SpotVMs". It now interprets `--use-spot auto` to mean that the framework decides whether or not to create a spot VM, whereas `--use-spot never` (default) unconditionally disables the use of spot VMs. Furthermore, to allow unconditional creation of spot VMs, pass `--use-spot always`. Finally, when running in CI, we pass `--use-spot auto`, at this time, to allow the framework to conditionally create spot VMs, i.e., based on current heuristics. [1] cockroachdb#116778 Epic: none Fixes: cockroachdb#77376 Release note: None

124258: roachtest: --use-spot now properly defaults to "no SpotVMs" r=DarrylWong,herkolategan,nameisbhaskar a=srosenberg In [1], the default of `UseSpotVM` was reinterpreted to mean the framework decides whether or not to create a spot VM. This may impede local (performance) testing when a potential preemption is unwelcome; i.e., may result in loss of work. This change reverts the original default of "no SpotVMs". It now interprets `--use-spot auto` to mean that the framework decides whether or not to create a spot VM, whereas `--use-spot never` (default) unconditionally disables the use of spot VMs. Furthermore, to allow unconditional creation of spot VMs, pass `--use-spot always`. Finally, when running in CI, we pass `--use-spot auto`, at this time, to allow the framework to conditionally create spot VMs, i.e., based on current heuristics. [1] #116778 Epic: none Fixes: #77376 Release note: None Co-authored-by: Stan Rosenberg <stan.rosenberg@gmail.com>

In [1], the default of `UseSpotVM` was reinterpreted to mean the framework decides whether or not to create a spot VM. This may impede local (performance) testing when a potential preemption is unwelcome; i.e., may result in loss of work. This change reverts the original default of "no SpotVMs". It now interprets `--use-spot auto` to mean that the framework decides whether or not to create a spot VM, whereas `--use-spot never` (default) unconditionally disables the use of spot VMs. Furthermore, to allow unconditional creation of spot VMs, pass `--use-spot always`. Finally, when running in CI, we pass `--use-spot auto`, at this time, to allow the framework to conditionally create spot VMs, i.e., based on current heuristics. [1] #116778 Epic: none Fixes: #77376 Release note: None

Fixes: cockroachdb#77376 Release note (ops change): --- security: add ttl metrics for certificate expiration Currently, cockroach only exposes point in time certificate expiration metrics. If the certificate is to expire 1 day from now, we expose a gauge `security.certificate.expiration.<cert-type>` which is the unix timestamp when it will expire. This PR also exposes a ttl metric `security.certificate.ttl.<cert-type>` so that consumers of this information can run operations based on their distance to certificate expiration without additional transformations. Additionally, this PR refactors how the expiration gauges are set, so that reads of the gauge directly reference the value of the certificate.

Currently, cockroach only exposes point in time certificate expiration metrics. If the certificate is to expire 1 day from now, we expose a gauge `security.certificate.expiration.<cert-type>` which is the unix timestamp when it will expire. This PR also exposes a ttl metric `security.certificate.ttl.<cert-type>` so that consumers of this information can run operations based on their distance to certificate expiration without additional transformations. Additionally, this PR refactors how the expiration gauges are set, so that reads of the gauge directly reference the value of the certificate. Epic: CRDB-40209 Fixes: cockroachdb#77376 Release note (ops change): new metrics which expose the ttl for various certificates

130110: ops: add ttl metrics for certificate expiration r=angles-n-daemons a=angles-n-daemons ops: add ttl metrics for certificate expiration Currently, cockroach only exposes point in time certificate expiration metrics. If the certificate is to expire 1 day from now, we expose a gauge `security.certificate.expiration.<cert-type>` which is the unix timestamp when it will expire. This PR also exposes a ttl metric `security.certificate.ttl.<cert-type>` so that consumers of this information can run operations based on their distance to certificate expiration without additional transformations. Additionally, this PR refactors how the expiration gauges are set, so that reads of the gauge directly reference the value of the certificate. Epic: CRDB-40209 Fixes: #77376 Release note (ops change): new metrics which expose the ttl for various certificates 130699: replica_rac2: remove a todo and add another one r=pav-kv a=sumeerbhola The first todo related to Processor.OnDescChangedLocked is obsolete, since we enqueue for ready processing. But ready procesing is not unconditional, hence the new todo. Epic: CRDB-37515 Release note: None Co-authored-by: angles-n-daemons <brian.dillmann@cockroachlabs.com> Co-authored-by: sumeerbhola <sumeer@cockroachlabs.com>

This PR adds the yaml configurations for the scale test PCR and Restore clusters Fixes: cockroachdb#77376 Release note: None

133796: drtprod: add create and destroy yamls for pcr and restore clusters r=nameisbhaskar a=vidit-bhat This PR adds the yaml configurations for the scale test PCR and Restore clusters Fixes: #77376 Release note: None 133850: sql/catalog: fix panic inside sequence expression parsing r=fqazi a=fqazi Previously, if the correct overloads were not found for sequence builtins it was possible for the server to panic. This could happen when rewriting a CREATE TABLE expression with an invalid sequence builtin call. To address this, this patch updates the sequence logic to return the error instead of panicking on it. Fixes: #133399 Release note (bug fix): Address a panic inside CREATE TABLE AS if sequence builtin expressions had invalid function overloads. Co-authored-by: Vidit Bhat <vidit.bhat@cockroachlabs.com> Co-authored-by: Faizan Qazi <faizan@cockroachlabs.com>

In rare cases (e.g. cockroachdb#77376), two transactions can get repeatedly deadlocked while trying to write to same key(s): one aborts the other, but before it can proceed, the other transaction has restarted and acquired a lock on the key again. This can result in the max transaction retries being exceeded without either transaction succeeding. This commit adds a backoff to the transaction retry loop in `db.Txn`, which will hopefully help one transaction slow down and let the other one commit. Fixes: cockroachdb#77376 Release note: None

jlinder added C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-dev-inf labels Mar 4, 2022

jlinder self-assigned this Mar 4, 2022

jlinder mentioned this issue Mar 11, 2022

lint: Add commit/PR body linter for epic and issue refs #77654

Closed

fqazi mentioned this issue Mar 23, 2023

workload/schemachanger: basic declarative schema changer support #99368

Merged

srosenberg mentioned this issue May 15, 2024

roachtest: --use-spot now properly defaults to "no SpotVMs" #124258

Merged

srosenberg mentioned this issue May 24, 2024

release-24.1: roachtest: --use-spot now properly defaults to "no SpotVMs" #124659

Merged

angles-n-daemons mentioned this issue Aug 19, 2024

base, licenseccl: Revise resolution strategy for LicenseTTL #129052

Merged

angles-n-daemons mentioned this issue Sep 4, 2024

ops: add ttl metrics for certificate expiration #130110

Merged

vidit-bhat added a commit to vidit-bhat/cockroach that referenced this issue Oct 30, 2024

drtprod: add create and destroy yamls for pcr and restore clusters

5bb882d

This PR adds the yaml configurations for the scale test PCR and Restore clusters Fixes: cockroachdb#77376 Release note: None

vidit-bhat mentioned this issue Oct 30, 2024

drtprod: add create and destroy yamls for pcr and restore clusters #133796

Merged

vidit-bhat added a commit to vidit-bhat/cockroach that referenced this issue Oct 30, 2024

drtprod: add create and destroy yamls for pcr and restore clusters

8256ac3

This PR adds the yaml configurations for the scale test PCR and Restore clusters Fixes: cockroachdb#77376 Release note: None

miraradeva mentioned this issue Nov 14, 2024

kv: add a backoff to the retry loop in db.Txn #135173

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

linter: Add PR check to encourage including issue or epic refs in the PR/commit messages #77376

linter: Add PR check to encourage including issue or epic refs in the PR/commit messages #77376

jlinder commented Mar 4, 2022 •

edited by exalate-issue-sync bot

Loading

linter: Add PR check to encourage including issue or epic refs in the PR/commit messages #77376

linter: Add PR check to encourage including issue or epic refs in the PR/commit messages #77376

Comments

jlinder commented Mar 4, 2022 • edited by exalate-issue-sync bot Loading

jlinder commented Mar 4, 2022 •

edited by exalate-issue-sync bot

Loading