feat: add PreservedBranches config for branchcleaner to preserve branches #23735

Colstuwjx · 2021-09-24T07:02:41Z

…hes.

k8s-ci-robot · 2021-09-24T07:02:48Z

Welcome @Colstuwjx!

It looks like this is your first PR to kubernetes/test-infra 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/test-infra has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2021-09-24T07:02:49Z

Hi @Colstuwjx. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

petr-muller

/ok-to-test

One concern is that the PR implements a global set of branches, but branch names are per-repo entities, so I think this config should allow per-repo (and possibly per-org) configuration.

prow/plugins/config.go

Colstuwjx · 2021-10-02T01:02:38Z

One concern is that the PR implements a global set of branches, but branch names are per-repo entities, so I think this config should allow per-repo (and possibly per-org) configuration.

OK, I'll change it.

Colstuwjx · 2021-10-08T09:13:58Z

One concern is that the PR implements a global set of branches, but branch names are per-repo entities, so I think this config should allow per-repo (and possibly per-org) configuration.

I've change to map[string][]string, and it allows per-repo or per-org configuration.

Colstuwjx · 2021-10-08T10:50:10Z

@petr-muller Could you help me clarify the CI issue, I don't know how to fix the bazel issue...

petr-muller · 2021-10-11T17:25:50Z

Looks like a bunch of generated content needs to be updated:

https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/test-infra/23735/pull-test-infra-bazel/1446405683212718080#1:build-log.txt%3A6633

ERROR: bazel rules out of date. Fix with:
  bazel run @io_k8s_repo_infra//hack:update-bazel

https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/test-infra/23735/pull-test-infra-bazel/1446405683212718080#1:build-log.txt%3A6856

ERROR: modules changed. Fix with:
  bazel run @io_k8s_repo_infra//hack:update-deps

https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/test-infra/23735/pull-test-infra-bazel/1446405683212718080#1:build-log.txt%3A6879

         If this is expected, re-run the tests with the UPDATE env var set to update the fixture:
        	UPDATE=true go test ./prow/plugins/... -run TestGenYamlDocs

Colstuwjx · 2021-10-12T02:32:41Z

Looks like a bunch of generated content needs to be updated:

Yes, I've been tried to fix it, but it seems doesn't work:

(testinfradev) ➜  test-infra git:(prow-branchcleaner-protect-branches) ./hack/update-bazel.sh
INFO: Repository py_deps instantiated at:
  no stack (--record_rule_instantiation_callstack not enabled)
Repository rule pip_import defined at:
  /private/var/tmp/_bazel_user/eb2106dd05673888d179bfdf510c291b/external/rules_python/python/pip.bzl:44:29: in <toplevel>
ERROR: An error occurred during the fetch of repository 'py_deps':
   pip_import failed:  (Traceback (most recent call last):
  File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/runpy.py", line 174, in _run_module_as_main
    "__main__", fname, loader, pkg_name)
  File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/runpy.py", line 72, in _run_code
    exec code in run_globals
  File "/private/var/tmp/_bazel_user/eb2106dd05673888d179bfdf510c291b/external/rules_python/tools/piptool.par/__main__.py", line 66, in <module>
  File "/private/var/tmp/_bazel_user/eb2106dd05673888d179bfdf510c291b/external/rules_python/tools/piptool.par/__main__.py", line 45, in extract_packages
  File "/System/Library/Frameworks/Python.framework/Versions/2.7/Extras/lib/python/pkg_resources/__init__.py", line 1286, in set_extraction_path
    "Can't change extraction path, files already extracted"
ValueError: Can't change extraction path, files already extracted
)
ERROR: no such package '@py_deps//': pip_import failed:  (Traceback (most recent call last):
  File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/runpy.py", line 174, in _run_module_as_main
    "__main__", fname, loader, pkg_name)
  File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/runpy.py", line 72, in _run_code
    exec code in run_globals
  File "/private/var/tmp/_bazel_user/eb2106dd05673888d179bfdf510c291b/external/rules_python/tools/piptool.par/__main__.py", line 66, in <module>
  File "/private/var/tmp/_bazel_user/eb2106dd05673888d179bfdf510c291b/external/rules_python/tools/piptool.par/__main__.py", line 45, in extract_packages
  File "/System/Library/Frameworks/Python.framework/Versions/2.7/Extras/lib/python/pkg_resources/__init__.py", line 1286, in set_extraction_path
    "Can't change extraction path, files already extracted"
ValueError: Can't change extraction path, files already extracted
)
INFO: Elapsed time: 18.586s
INFO: 0 processes.
FAILED: Build did NOT complete successfully (0 packages loaded)
FAILED: Build did NOT complete successfully (0 packages loaded)

Colstuwjx · 2021-10-12T03:00:43Z

Update: I've been installed python27 on macOS, and alias it with alias python="/usr/local/bin/python2", now it throws an new error:

INFO: Analyzed target @io_k8s_repo_infra//hack:update-bazel (1 packages loaded, 348 targets configured).
INFO: Found 1 target...
ERROR: /private/var/tmp/_bazel_user/eb2106dd05673888d179bfdf510c291b/external/com_google_protobuf/BUILD:120:11: C++ compilation of rule '@com_google_protobuf//:protobuf_lite' failed (Exit 1) cc_wrapper.sh failed: error executing command external/local_config_cc/cc_wrapper.sh -U_FORTIFY_SOURCE -fstack-protector -Wall -Wthread-safety -Wself-assign -fcolor-diagnostics -fno-omit-frame-pointer -g0 -O2 '-D_FORTIFY_SOURCE=1' -DNDEBUG ... (remaining 33 argument(s) skipped)

Use --sandbox_debug to see verbose messages from the sandbox
external/com_google_protobuf/src/google/protobuf/any_lite.cc:56:14: error: non-constexpr declaration of 'AnyMetadata' follows constexpr declaration
AnyMetadata::AnyMetadata(UrlType* type_url, ValueType* value)
             ^
/usr/local/include/google/protobuf/any.h:63:13: note: previous declaration is here
  constexpr AnyMetadata(UrlType* type_url, ValueType* value)
            ^
external/com_google_protobuf/src/google/protobuf/any_lite.cc:59:19: error: return type of out-of-line definition of 'google::protobuf::internal::AnyMetadata::InternalPackFrom' differs from that in the declaration
void AnyMetadata::InternalPackFrom(const MessageLite& message,
~~~~              ^
/usr/local/include/google/protobuf/any.h:111:8: note: previous declaration is here
  bool InternalPackFrom(const MessageLite& message,
  ~~~~ ^
external/com_google_protobuf/src/google/protobuf/any_lite.cc:62:14: error: no member named 'SetNoArena' in 'google::protobuf::internal::ArenaStringPtr'
  type_url_->SetNoArena(&::google::protobuf::internal::GetEmptyString(),
  ~~~~~~~~~  ^
external/com_google_protobuf/src/google/protobuf/any_lite.cc:64:37: error: no member named 'MutableNoArena' in 'google::protobuf::internal::ArenaStringPtr'
  message.SerializeToString(value_->MutableNoArena(
                            ~~~~~~  ^
external/com_google_protobuf/src/google/protobuf/any_lite.cc:73:43: error: no member named 'GetNoArena' in 'google::protobuf::internal::ArenaStringPtr'
  return message->ParseFromString(value_->GetNoArena());
                                  ~~~~~~  ^
external/com_google_protobuf/src/google/protobuf/any_lite.cc:91:15: error: no member named 'GetNoArena' in 'google::protobuf::internal::ArenaStringPtr'
  return ptr->GetNoArena();
         ~~~  ^
6 errors generated.
Target @io_k8s_repo_infra//hack:update-bazel failed to build
Use --verbose_failures to see the command lines of failed build steps.
INFO: Elapsed time: 2.858s, Critical Path: 2.54s
INFO: 0 processes.
FAILED: Build did NOT complete successfully
FAILED: Build did NOT complete successfully

I have no idea about how to fix this issue..

Colstuwjx · 2021-10-12T06:49:15Z

Hi @petr-muller

Thanks for your reminder, I've met issue protobuf 5376 and I've fixed it.

Now it's ready for review, PTAL!

petr-muller

Last thing, then I think this looks good!

prow/plugins/config.go

Colstuwjx · 2021-10-13T00:57:51Z

@petr-muller Updated! PTAL!

Colstuwjx · 2021-10-18T07:25:16Z

/cc @cjwagner @stevekuznetsov Could you help me review this PR? Many thanks!!!

cjwagner

Looks good to me, but please also update the PluginHelp information for the plugin now that it is configurable.

cjwagner · 2021-10-18T23:19:14Z

prow/plugins/config.go

+	if _, existsRepo := b.PreservedBranches[fullRepoName]; existsRepo {
+		for _, pb := range b.PreservedBranches[fullRepoName] {
+			if branch == pb {
+				return true
+			}
+		}
+	}


nit: Go makes it safe to iterate over nil slices. So both of the repo check and the org check can drop the outer conditional.

Suggested change

if _, existsRepo := b.PreservedBranches[fullRepoName]; existsRepo {

for _, pb := range b.PreservedBranches[fullRepoName] {

if branch == pb {

return true

}

}

}

for _, pb := range b.PreservedBranches[fullRepoName] {

if branch == pb {

return true

}

}

cjwagner · 2021-10-18T23:25:53Z

prow/plugins/branchcleaner/branchcleaner.go

@@ -41,26 +41,31 @@ func helpProvider(config *plugins.Configuration, _ []config.OrgRepo) (*pluginhel
 }


Please also update the helpProvider to include include Config now that this plugin has configurable behavior. e.g.
and consider adding the YAML config snippet like this:

test-infra/prow/plugins/retitle/retitle.go

Lines 54 to 61 in f7e21a3

yamlSnippet, err := plugins.CommentMap.GenYaml(&plugins.Configuration{

Retitle: plugins.Retitle{

AllowClosedIssues: true,

},

})

if err != nil {

logrus.WithError(err).Warnf("cannot generate comments for %s plugin", pluginName)

}

Full details about PluginHelp.

Thanks for explanation. I've been tried to change the plugin helper content, but I found the prow/plugins/plugin-config-documented.yaml didn't change.

What's more, I have two confusions after you clarified the plugin help:

BranchCleaner is a passive plugin and don't give comment command, rather than lgtm or retitle, and I'm confused about how it could be configured, there are two styles of configuration formats:

# format 1 triggers: - repos: - org_a - repos: - org_a/repo_a1 only_org_members: true # format 2 plugins: org_a: plugins: - approve # Allow OWNERS to /approve

We can see, format 1 is static config, and it could set different config options for each org/repo set. And format 2 is a list of plugins, and each org/repo has a list of configured plugins with their config options. In format 2, it seems a little-bit redundant if we set map[repo]config inside the list, e.g.

plugins: org_a: plugins: - branch_cleaner: preserved_branches: "org_a": ["master"] "org_a/repo_a1": ["master"]

The nested configuration looks so bad, right? I'm wondering if we need to change the config.BranchCleaner config struct, and turn to use format 1. I'm also confused about these two formats, which one is suitable to BranchCleaner such plugin, why?

As I mentioned, I've been tried UPDATE=true go test ./prow/plugins/... -run TestGenYamlDocs and the yaml content didn't change. So what's the process to update and plugin helper and its document yaml ? Besides the annotations you given, I can't find any document about this process :(

Could you help me clarify these confusions?
Thanks!

I've been tried to change the plugin helper content, but I found the prow/plugins/plugin-config-documented.yaml didn't change.

It is not expected to, that file is generated based on an empty plugins.Configuration struct and doesn't involve PluginHelp.
PluginHelp information is displayed via Prow's front end. Try clicking on the "Details" button for the "Approve" plugin card on this page to see an example of the configuration section and the example snippet section. https://prow.k8s.io/plugins?repo=kubernetes%2Ftest-infra Notice that the configuration section depends on the repo selected, but the snippet is just a static example.

BranchCleaner is a passive plugin and don't give comment command, rather than lgtm or retitle, and I'm confused about how it could be configured, there are two styles of configuration formats

Passive plugins can have configuration, the addition you made to the plugins.Configuration struct is exactly what I'm talking about. The configuration format you already added looks good and is consistent with other plugin configurations, so no need to change it.

cjwagner · 2021-10-18T23:28:47Z

prow/plugins/plugin-config-documented.yaml

+    # branches in this allow map would be exempt from branch gc
+    # even if the branches are already merged into the target branch
+    preserved_branches:
+        "": null


Populating the PluginHelp.Snippet field as I mentioned in the other comment will result in a commented version of the YAML config like this, but with the currently configured values.

Sorry, this was confusing and not quite accurate. I didn't mean to suggest that this file would update, just that the commented config snippet would look similar to this due to being generated with the same mechanism. I was wrong about the config snippet showing the currently configured values, it should just be a static example config.

cjwagner

Thanks for adding the PluginHelp info!

cjwagner · 2021-10-19T21:38:26Z

prow/plugins/plugin-config-documented.yaml

+    # branches in this allow map would be exempt from branch gc
+    # even if the branches are already merged into the target branch
+    preserved_branches:
+        "": null


Sorry, this was confusing and not quite accurate. I didn't mean to suggest that this file would update, just that the commented config snippet would look similar to this due to being generated with the same mechanism. I was wrong about the config snippet showing the currently configured values, it should just be a static example config.

cjwagner · 2021-10-19T21:47:15Z

prow/plugins/branchcleaner/branchcleaner.go

@@ -41,26 +41,31 @@ func helpProvider(config *plugins.Configuration, _ []config.OrgRepo) (*pluginhel
 }


I've been tried to change the plugin helper content, but I found the prow/plugins/plugin-config-documented.yaml didn't change.

It is not expected to, that file is generated based on an empty plugins.Configuration struct and doesn't involve PluginHelp.
PluginHelp information is displayed via Prow's front end. Try clicking on the "Details" button for the "Approve" plugin card on this page to see an example of the configuration section and the example snippet section. https://prow.k8s.io/plugins?repo=kubernetes%2Ftest-infra Notice that the configuration section depends on the repo selected, but the snippet is just a static example.

BranchCleaner is a passive plugin and don't give comment command, rather than lgtm or retitle, and I'm confused about how it could be configured, there are two styles of configuration formats

Passive plugins can have configuration, the addition you made to the plugins.Configuration struct is exactly what I'm talking about. The configuration format you already added looks good and is consistent with other plugin configurations, so no need to change it.

k8s-ci-robot · 2021-10-19T21:48:14Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cjwagner, Colstuwjx, petr-muller

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~prow/OWNERS~~ [cjwagner]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

feat: add ProtectedBranches config for branchcleaner to protect branc…

e31a2e6

…hes.

k8s-ci-robot requested review from petr-muller and stevekuznetsov September 24, 2021 07:03

petr-muller reviewed Sep 27, 2021

View reviewed changes

prow/plugins/config.go Outdated Show resolved Hide resolved

prow/plugins/config.go Outdated Show resolved Hide resolved

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 27, 2021

Colstuwjx added 3 commits October 2, 2021 09:07

fix: revise the word.

f92f24d

fix: change to preservedBranches and support config towards each repo.

953d155

fix: check per-repo or per-org.

65753cf

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 8, 2021

fix: since we only check same repo, removed head vars.

960b58b

Colstuwjx changed the title ~~feat: add ProtectedBranches config for branchcleaner to protect branches~~ feat: add PreservedBranches config for branchcleaner to preserve branches Oct 12, 2021

fix: fixed bazel and prow plugin issues.

5edbf85

petr-muller reviewed Oct 12, 2021

View reviewed changes

prow/plugins/config.go Outdated Show resolved Hide resolved

fix: add how to config repo, org/repo document.

24b5efa

petr-muller approved these changes Oct 14, 2021

View reviewed changes

k8s-ci-robot assigned petr-muller Oct 14, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 14, 2021

cjwagner reviewed Oct 18, 2021

View reviewed changes

fix: optimize code and fillin plugin helper.

9df5d04

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 19, 2021

cjwagner approved these changes Oct 19, 2021

View reviewed changes

k8s-ci-robot assigned cjwagner Oct 19, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 19, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 19, 2021

k8s-ci-robot merged commit b0788a9 into kubernetes:master Oct 19, 2021

k8s-ci-robot added this to the v1.23 milestone Oct 19, 2021

Colstuwjx deleted the prow-branchcleaner-protect-branches branch October 19, 2021 22:44

Colstuwjx mentioned this pull request Oct 22, 2021

[ prow ] need to squash commits for test-infra repo itself by default #24096

Closed

hvaara mentioned this pull request Oct 9, 2022

Prow: Should branchcleaner be configurable not to clean certain branches? #15224

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add PreservedBranches config for branchcleaner to preserve branches #23735

feat: add PreservedBranches config for branchcleaner to preserve branches #23735

Colstuwjx commented Sep 24, 2021

k8s-ci-robot commented Sep 24, 2021

k8s-ci-robot commented Sep 24, 2021

petr-muller left a comment

Colstuwjx commented Oct 2, 2021

Colstuwjx commented Oct 8, 2021

Colstuwjx commented Oct 8, 2021

petr-muller commented Oct 11, 2021

Colstuwjx commented Oct 12, 2021

Colstuwjx commented Oct 12, 2021

Colstuwjx commented Oct 12, 2021

petr-muller left a comment

Colstuwjx commented Oct 13, 2021

Colstuwjx commented Oct 18, 2021

cjwagner left a comment

cjwagner Oct 18, 2021

cjwagner Oct 18, 2021

Colstuwjx Oct 19, 2021 •

edited

Loading

cjwagner Oct 19, 2021

cjwagner Oct 18, 2021

cjwagner Oct 19, 2021

cjwagner left a comment

cjwagner Oct 19, 2021

cjwagner Oct 19, 2021

k8s-ci-robot commented Oct 19, 2021

		@@ -41,26 +41,31 @@ func helpProvider(config plugins.Configuration, _ []config.OrgRepo) (pluginhel
		}

	yamlSnippet, err := plugins.CommentMap.GenYaml(&plugins.Configuration{
	Retitle: plugins.Retitle{
	AllowClosedIssues: true,
	},
	})
	if err != nil {
	logrus.WithError(err).Warnf("cannot generate comments for %s plugin", pluginName)
	}

feat: add PreservedBranches config for branchcleaner to preserve branches #23735

feat: add PreservedBranches config for branchcleaner to preserve branches #23735

Conversation

Colstuwjx commented Sep 24, 2021

k8s-ci-robot commented Sep 24, 2021

k8s-ci-robot commented Sep 24, 2021

petr-muller left a comment

Choose a reason for hiding this comment

Colstuwjx commented Oct 2, 2021

Colstuwjx commented Oct 8, 2021

Colstuwjx commented Oct 8, 2021

petr-muller commented Oct 11, 2021

Colstuwjx commented Oct 12, 2021

Colstuwjx commented Oct 12, 2021

Colstuwjx commented Oct 12, 2021

petr-muller left a comment

Choose a reason for hiding this comment

Colstuwjx commented Oct 13, 2021

Colstuwjx commented Oct 18, 2021

cjwagner left a comment

Choose a reason for hiding this comment

cjwagner Oct 18, 2021

Choose a reason for hiding this comment

cjwagner Oct 18, 2021

Choose a reason for hiding this comment

Colstuwjx Oct 19, 2021 • edited Loading

Choose a reason for hiding this comment

cjwagner Oct 19, 2021

Choose a reason for hiding this comment

cjwagner Oct 18, 2021

Choose a reason for hiding this comment

cjwagner Oct 19, 2021

Choose a reason for hiding this comment

cjwagner left a comment

Choose a reason for hiding this comment

cjwagner Oct 19, 2021

Choose a reason for hiding this comment

cjwagner Oct 19, 2021

Choose a reason for hiding this comment

k8s-ci-robot commented Oct 19, 2021

Colstuwjx Oct 19, 2021 •

edited

Loading