Update .asf.yaml to protect release branches #14226

michaeljmarshall · 2022-02-10T23:54:35Z

Motivation

Many of our current Pulsar branches are not protected. They are vulnerable to accidental force pushes. We should not allow for any modification of history on our release branches.

Further, it is important to update this check because we currently have several status checks that are not required. If a committer were not paying close attention, they could accidentally merge a PR that is not passing all tests. Here is an example: #14158 (comment).

I followed the instructions here https://cwiki.apache.org/confluence/display/INFRA/Git+-+.asf.yaml+features#Git.asf.yamlfeatures-BranchProtection when drafting this PR.

Modifications

Update the .asf.yaml file.
** There are instructions here: https://cwiki.apache.org/confluence/display/INFRA/Git+-+.asf.yaml+features#Git.asf.yamlfeatures-BranchProtection

Verifying this change

I opened this ASF Infra Jira ticket https://issues.apache.org/jira/browse/INFRA-22833 to discuss the current protections for our branches.

Checking with ASF infra is important because otherwise we could overwrite current protections:

NB (1): Enabling any of the above checks overrides what you may have set previously, so you'll need to add all the existing checks to your .asf.yaml to reproduce any that Infra set manually for you.

Does this pull request potentially affect one of the following parts:

This PR only affects the GitHub repo management.

Documentation

We don't need documentation for this feature. The git history should be sufficient.

.asf.yaml

michaeljmarshall · 2022-02-11T00:02:33Z

.asf.yaml

+          - CI - Cancel duplicate workflows
+          - CI - CPP, Python Tests
+          - CI - CPP build on CentOS 7
+          - CI - CPP build on Windows
+          - Auto Labeling
+          - CI - Go Functions style check
+          - CI - Go Functions Tests
+          - CI - Integration - Backwards Compatibility
+          - CI - Integration - Cli
+          - CI - Integration - Function & IO
+          - CI - Integration - Messaging
+          - CI - Integration - Process
+          - CI - Integration - Pulsar-IO Sinks and Sources
+          - CI - Integration - Pulsar-IO Oracle Source
+          - CI - Integration - Schema
+          - CI - Integration - Sql
+          - CI - Integration - Standalone
+          - CI - Integration - Thread
+          - CI - Integration - Tiered FileSystem
+          - CI - Integration - Tiered JCloud
+          - CI - Integration - Transaction
+          - CI - Misc
+          - CI - Maven Dependency Cache Update
+          - CI - Pulsar Website build
+          - Pulsar Bot
+          - CI - Python - Build 3.9 client
+          - CI - Shade - Test
+          - CI - Unit
+          - CI - Unit - Brokers - Broker Group
+          - CI - Unit - Brokers - Client Api
+          - CI - Unit - Brokers - Client Impl
+          - CI - Unit - Broker - JDK8
+          - CI - Unit - Brokers - Other
+          - CI - Unit - Proxy


Please let me know if any of these CI jobs are not supposed to be required. Thanks!

.asf.yaml

zymap · 2022-02-11T02:16:54Z

.asf.yaml

+          - CI - CPP build on CentOS 7
+          - CI - CPP build on Windows
+          - Auto Labeling


I think this can be removed, it's not a check.

zymap · 2022-02-11T02:18:51Z

.asf.yaml

+          - CI - Pulsar Website build
+          - Pulsar Bot


Those three can remove I think

lhotari · 2022-02-11T08:55:18Z

btw. it's possible to find out currently protected branches with this GitHub API call

❯ curl -s https://api.github.com/repos/apache/pulsar/branches |jq -r '.[] | select(.protected) | .name'
asf-site
branch-1.15
branch-1.16
branch-1.17
branch-1.18
master

lhotari · 2022-02-11T09:01:17Z

Getting current required status checks:

❯ curl -s -H 'Accept: application/vnd.github.v3+json' https://api.github.com/repos/apache/pulsar/branches/master | jq .protection
{
  "enabled": true,
  "required_status_checks": {
    "enforcement_level": "non_admins",
    "contexts": [
      "License check",
      "backwards-compatibility",
      "cli",
      "cpp-tests",
      "function-state",
      "messaging",
      "process",
      "schema",
      "sql",
      "standalone",
      "thread",
      "tiered-filesystem",
      "tiered-jcloud",
      "unit-tests"
    ],
    "checks": [
      {
        "context": "License check",
        "app_id": null
      },
      {
        "context": "backwards-compatibility",
        "app_id": null
      },
      {
        "context": "cli",
        "app_id": null
      },
      {
        "context": "cpp-tests",
        "app_id": null
      },
      {
        "context": "function-state",
        "app_id": null
      },
      {
        "context": "messaging",
        "app_id": null
      },
      {
        "context": "process",
        "app_id": null
      },
      {
        "context": "schema",
        "app_id": null
      },
      {
        "context": "sql",
        "app_id": null
      },
      {
        "context": "standalone",
        "app_id": null
      },
      {
        "context": "thread",
        "app_id": null
      },
      {
        "context": "tiered-filesystem",
        "app_id": null
      },
      {
        "context": "tiered-jcloud",
        "app_id": null
      },
      {
        "context": "unit-tests",
        "app_id": null
      }
    ]
  }
}

GitHub API docs: https://docs.github.com/en/rest/reference/branches#get-branch-protection

lhotari · 2022-02-11T09:07:12Z

.asf.yaml

+        # found in the "name:" field of ./github/workflows/*.yaml files
+        # Note: here is the list of intentionally excluded contexts:


This isn't correct information. See #14226 (comment) for the current information. The contexts gets passed as-is in the asfgit python code. I'll check where to get the correct context names.

list of context names: #14226 (comment) .

The context name is the name of the job in the workflow yaml file. It's not the name of the workflow.

Thank you for finding this. I struggled to find documentation on this piece, and determined it was the name based on the the way the GitHub UI takes input. This makes much more sense.

lhotari · 2022-02-11T09:28:30Z

Here's a list of the correct context names for required_status_checks:

❯ curl -s "https://api.github.com/repos/apache/pulsar/commits/$(curl -s https://api.github.com/repos/apache/pulsar/pulls/14147 | jq -r .head.sha)/check-runs" |jq -r '.check_runs | .[] | .name'
labeling
action-runner
unit-tests
unit-tests
pulsar-ci-test (group3)
pulsar-ci-test (group2)
pulsar-ci-test (group1)
cli
unit-tests
unit-tests
tiered-filesystem
owasp-dep-check
sql
messaging
shade-check
process
tiered-jcloud
function-state
schema
transaction
cpp-tests
pulsar-io
thread
License check
standalone
build
unit-tests
backwards-compatibility
unit-tests
pulsar-io

Some workflows have duplicate names. I guess that's fine, but the only issue it causes it that you cannot distinguish a specific workflow.

lhotari · 2022-02-11T09:58:11Z

Listing job names, requires yq (brew install yq):

for f in .github/workflows/*.yaml; do FILE=$f yq eval -o j '.jobs | to_entries | {"file": env(FILE),"id":.[].key, "name":.[].value.name}' $f; done

The name of the job is the id if the name is empty or unspecified. for example

{
  "file": ".github/workflows/ci-license.yaml",
  "id": "license-check",
  "name": "License check"
}

and in comparison

{
  "file": ".github/workflows/ci-owasp-dep-check.yaml",
  "id": "owasp-dep-check",
  "name": null
}

matrix jobs are an exception. if the name isn't specified, the matrix variable will be part of the name, for example for file .github/workflows/ci-unit-broker-broker-gp.yaml, the generated names are:

pulsar-ci-test (group3)
pulsar-ci-test (group2)
pulsar-ci-test (group1)

lhotari

see https://github.com/apache/pulsar/pull/14226/files#r804464516

michaeljmarshall · 2022-02-11T17:42:25Z

Some workflows have duplicate names. I guess that's fine, but the only issue it causes it that you cannot distinguish a specific workflow.

@lhotari - this could be a feature in the sense that all duplicate names get the same treatment. For example, if all "unit tests" are named the same, they'll all be required. In the GitHub UI, they are visibly different because the top level name field is associated with the check. I left the duplicates in place for now. Please take a look and let me know what you think. Thanks.

dave2wave

I'm told that while wildcards are not possible globs may be possible, but I think that would need to be tested with the GitHub API

michaeljmarshall · 2022-02-11T21:30:06Z

@dave2wave - that makes sense to me. I'd expect to just need branch-*. I will test it out and update this PR if that works.

michaeljmarshall · 2022-02-11T21:47:38Z

@dave2wave - as far as I can tell, it's not possible to set wildcard protections using the REST API. It's only possible with the GraphQL API. @lhotari found the GitBox code https://github.com/apache/infrastructure-puppet/blob/deployment/modules/gitbox/files/asfgit/asfyaml.py#L278-L302, and it looks to me like it currently uses the REST API.

Source: https://github.saobby.my.eu.orgmunity/t/rest-api-v3-wildcard-branch-protection/13593/13

EDIT: add wildcard in first sentence.

dave2wave · 2022-02-11T23:05:19Z

@michaeljmarshall The API used by .asf.yaml very likely predates the GraphQL. Changing the API would be a larger project and impacts every project that uses .asf.yaml which is a large proportion of ASF projects. If anyone is interested in helping ASF Infra I can connect you. (Note that one given is that everything with Infra must be Python and for templates EZT is preferred.)

michaeljmarshall · 2022-02-11T23:09:44Z

@dave2wave - sorry, I forgot a word in my last message. I meant to say it's not possible to set wildcard permissions via the REST API. This will work as is. We will just have to update the release process to include adding branch protections to new branches.

dave2wave · 2022-02-11T23:19:39Z

Sure. If a volunteer wished to implement wildcard branch protection update then you would start by rewriting https://github.com/apache/infrastructure-puppet/blob/deployment/modules/gitbox/files/asfgit/asfyaml.py#L257-L391

.asf.yaml

michaeljmarshall · 2022-02-15T16:18:43Z

@lhotari - can you take another look? Thanks.

This addition ensures that `branch-2.10` is protected from force pushes. See #14226 for more information.

* Update .asf.yaml to protect release branches * Remove protections and required runs based on review * Fix context names and document changes

This addition ensures that `branch-2.10` is protected from force pushes. See apache#14226 for more information.

michaeljmarshall added area/build doc-not-needed Your PR changes do not impact docs labels Feb 10, 2022

michaeljmarshall requested review from merlimat, lhotari, sijie, eolivelli and codelipenghui February 10, 2022 23:54

michaeljmarshall self-assigned this Feb 10, 2022

michaeljmarshall requested a review from dave2wave February 10, 2022 23:54

dlg99 reviewed Feb 11, 2022

View reviewed changes

.asf.yaml Outdated Show resolved Hide resolved

michaeljmarshall commented Feb 11, 2022

View reviewed changes

merlimat reviewed Feb 11, 2022

View reviewed changes

.asf.yaml Outdated Show resolved Hide resolved

zymap reviewed Feb 11, 2022

View reviewed changes

lhotari reviewed Feb 11, 2022

View reviewed changes

lhotari requested changes Feb 11, 2022

View reviewed changes

michaeljmarshall added 3 commits February 11, 2022 10:22

Update .asf.yaml to protect release branches

45a5d49

Remove protections and required runs based on review

20bd7a6

Fix context names and document changes

c1e877e

michaeljmarshall force-pushed the protect-our-branches branch from a11f086 to c1e877e Compare February 11, 2022 17:40

dave2wave approved these changes Feb 11, 2022

View reviewed changes

lhotari reviewed Feb 12, 2022

View reviewed changes

.asf.yaml Show resolved Hide resolved

michaeljmarshall requested a review from lhotari February 14, 2022 18:09

lhotari approved these changes Feb 15, 2022

View reviewed changes

lhotari merged commit aa9f42c into apache:master Feb 15, 2022

michaeljmarshall deleted the protect-our-branches branch February 15, 2022 16:36

michaeljmarshall mentioned this pull request Feb 17, 2022

Protect branch-2.10 from force pushes #14334

Merged

michaeljmarshall added a commit that referenced this pull request Feb 17, 2022

Protect branch-2.10 from force pushes (#14334)

a9536f8

This addition ensures that `branch-2.10` is protected from force pushes. See #14226 for more information.

michaeljmarshall mentioned this pull request Feb 23, 2022

Update .asf.yaml: keep stale reviews on PRs targeting master #14425

Merged

Nicklee007 pushed a commit to Nicklee007/pulsar that referenced this pull request Apr 20, 2022

Protect branch-2.10 from force pushes (apache#14334)

d9e909f

This addition ensures that `branch-2.10` is protected from force pushes. See apache#14226 for more information.

michaeljmarshall mentioned this pull request Aug 9, 2022

Update .asf.yaml to protect branch-2.11 #17002

Merged

1 task

lhotari mentioned this pull request Oct 16, 2024

[improve][ci] Prevent git force push to more recent maintenance branches #23464

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update .asf.yaml to protect release branches #14226

Update .asf.yaml to protect release branches #14226

michaeljmarshall commented Feb 10, 2022

michaeljmarshall Feb 11, 2022

zymap Feb 11, 2022

zymap Feb 11, 2022

lhotari commented Feb 11, 2022

lhotari commented Feb 11, 2022 •

edited

Loading

lhotari Feb 11, 2022

lhotari Feb 11, 2022

michaeljmarshall Feb 11, 2022

lhotari commented Feb 11, 2022

lhotari commented Feb 11, 2022 •

edited

Loading

lhotari left a comment

michaeljmarshall commented Feb 11, 2022

dave2wave left a comment •

edited

Loading

michaeljmarshall commented Feb 11, 2022

michaeljmarshall commented Feb 11, 2022 •

edited

Loading

dave2wave commented Feb 11, 2022

michaeljmarshall commented Feb 11, 2022

dave2wave commented Feb 11, 2022

michaeljmarshall commented Feb 15, 2022

		# found in the "name:" field of ./github/workflows/*.yaml files
		# Note: here is the list of intentionally excluded contexts:

Update .asf.yaml to protect release branches #14226

Update .asf.yaml to protect release branches #14226

Conversation

michaeljmarshall commented Feb 10, 2022

Motivation

Modifications

Verifying this change

Does this pull request potentially affect one of the following parts:

Documentation

michaeljmarshall Feb 11, 2022

Choose a reason for hiding this comment

zymap Feb 11, 2022

Choose a reason for hiding this comment

zymap Feb 11, 2022

Choose a reason for hiding this comment

lhotari commented Feb 11, 2022

lhotari commented Feb 11, 2022 • edited Loading

lhotari Feb 11, 2022

Choose a reason for hiding this comment

lhotari Feb 11, 2022

Choose a reason for hiding this comment

michaeljmarshall Feb 11, 2022

Choose a reason for hiding this comment

lhotari commented Feb 11, 2022

lhotari commented Feb 11, 2022 • edited Loading

lhotari left a comment

Choose a reason for hiding this comment

michaeljmarshall commented Feb 11, 2022

dave2wave left a comment • edited Loading

Choose a reason for hiding this comment

michaeljmarshall commented Feb 11, 2022

michaeljmarshall commented Feb 11, 2022 • edited Loading

dave2wave commented Feb 11, 2022

michaeljmarshall commented Feb 11, 2022

dave2wave commented Feb 11, 2022

michaeljmarshall commented Feb 15, 2022

lhotari commented Feb 11, 2022 •

edited

Loading

lhotari commented Feb 11, 2022 •

edited

Loading

dave2wave left a comment •

edited

Loading

michaeljmarshall commented Feb 11, 2022 •

edited

Loading