Add support of excluded_repos for plugins #20707

matthyx · 2021-02-02T15:30:30Z

Fixes #20631
/cc @chaodaiG

matthyx · 2021-02-02T15:31:16Z

I need to add test cases for the new format, and fix some CI checks, but could you check already if that would do it?

matthyx · 2021-02-02T15:33:23Z

This is the first PR where I need to support both old and new config styles... after some time we can simplify it by ditching the old format.

chaodaiG · 2021-02-02T16:27:46Z

Would worth mentioning in https://github.com/kubernetes/test-infra/blob/master/prow/ANNOUNCEMENTS.md as well I think

chaodaiG · 2021-02-04T22:09:35Z

I need to add test cases for the new format, and fix some CI checks, but could you check already if that would do it?

Sorry overlooked this comment last time. This looks exactly like what's in my mind!

matthyx · 2021-02-17T08:25:56Z

prow/plugins/config.go

@@ -817,9 +853,15 @@ func (c *Configuration) EnabledReposForPlugin(plugin string) (orgs, repos []stri
 				repos = append(repos, repo)
 			} else {
 				orgs = append(orgs, repo)
+				orgExceptions[repo] = sets.NewString(plugins.ExcludedRepos...)


I am not sure orgExceptions is the right way to go... do you have an idea @alvaroaleman ?

My feeling is that this is very explicit, like orgA/repoB, so a slice of repos should do?

I am just trying to integrate with the existing mechanisms

matthyx · 2021-02-17T16:16:09Z

@chaodaiG PTAL

chaodaiG · 2021-02-17T16:49:17Z

prow/plugins/config_test.go

+	}
+	testCases := []struct {
+		name          string
+		orgs          []string


nit: rename the wanted output to make tests better readable, such as:

name string wantOrgs []string wantRepos []string wantExcludedRepos map[string]sets.String

chaodaiG · 2021-02-17T17:39:24Z

prow/plugins/config.go

+	return newPlugins
+}
+
+func ToRawMessage(plugins interface{}) json.RawMessage {


Is this used?

yes in all tests, as during the migration time we need to use a json.RawMessage to allow deserialization with our own code until we completely switch to the new form

chaodaiG · 2021-02-17T17:48:06Z

prow/plugins/config.go

+	if err := yaml.Unmarshal(c.PluginsRaw, &oldPlugins); err != nil {
+		var newPlugins map[string]OrgPlugins
+		if err := yaml.Unmarshal(c.PluginsRaw, &newPlugins); err != nil {
+			logrus.WithError(err).Error("error unmarshalling Plugins config to oldPlugins and newPlugins formats")


I feel that we might want to return an error here, if we can't unmarshal plugins config then hook is almost not functioning.

Also a nit, you can return earlier to avoid nested if else loop. for example:

if err := yaml.Unmarshal(c.PluginsRaw, &oldPlugins); err == nil { logrus.Warn("plugins declaration uses a deprecated config style, please migrate it") return oldToNewPlugins(oldPlugins), nil } var newPlugins map[string]OrgPlugins err := yaml.Unmarshal(c.PluginsRaw, &newPlugins) return newPlugins, err

I see, but then what do I do with this error in the caller side?

I think checkconfig need to catch this error and not letting the change go through. And for hook I would prefer to let it crash, with an error message asking for running checkconfig

This custom unmarshaling is just for the transition period, to let people migrate from old to new format... otherwise such error is thrown when the yaml file is parsed, so maybe I should validate at that very beginning and not every time I need to access plugins?

Ok, I will do like ConfigUpdater and redefine my own unmarshall method...

chaodaiG · 2021-02-17T17:55:54Z

prow/plugins/config.go

 			}
 		}
 	}
+	// if org/repo has explicitly plugin enabled, remove its exception


This is slightly confusing, I guess you meant:

- orgA plugins: - common excluded: - repoB - orgA/repoB plugins: - common

We might need more comments here

yes that's what I meant, alright will rephrase

chaodaiG · 2021-02-17T18:00:19Z

prow/plugins/config.go

@@ -817,9 +853,15 @@ func (c *Configuration) EnabledReposForPlugin(plugin string) (orgs, repos []stri
 				repos = append(repos, repo)
 			} else {
 				orgs = append(orgs, repo)
+				orgExceptions[repo] = sets.NewString(plugins.ExcludedRepos...)


My feeling is that this is very explicit, like orgA/repoB, so a slice of repos should do?

chaodaiG · 2021-02-17T18:02:58Z

prow/plugins/plugins.go

-	plugins = append(plugins, pa.configuration.Plugins[owner]...)
-	plugins = append(plugins, pa.configuration.Plugins[fullName]...)
+	plugins = append(plugins, pa.configuration.Plugins()[owner].Plugins...)
+	plugins = append(plugins, pa.configuration.Plugins()[fullName].Plugins...)


Shouldn't we process excluded repos here?

chaodaiG · 2021-02-18T14:38:20Z

/cc @alvaroaleman

chaodaiG

Overall looks awesome to me. Left one last nit

/hold
In case you want to address

chaodaiG · 2021-02-19T15:35:42Z

prow/plugins/plugins_test.go

+		pa := ConfigAgent{configuration: &Configuration{Plugins: OldToNewPlugins(tc.pluginMap)}}
+
+		plugins := pa.getPlugins(tc.owner, tc.repo)
+		if len(plugins) != len(tc.expectedPlugins) {


Is it possible to use cmp.Diff instead? Same below

matthyx · 2021-02-19T15:53:27Z

In case you want to address

Squashed commits as well. Thanks for your help!

chaodaiG

Looks awesome! Thank you for the contribution!

@michelle192837 and @alvaroaleman for awareness

chaodaiG

Just noticed that there is still a hold on the PR, left a comment. No big deal, so please feel free address it in current PR or in future PR

chaodaiG · 2021-02-19T18:37:12Z

prow/plugins/config_test.go

+ - pluginOnlyForRepoB
+`)
+	var p Plugins
+	err := yaml.Unmarshal(badPluginsYaml, &p)


Sorry I must haven't got enough coffee for the morning, didn't notice this test didn't use UnmarshalJSON method. Please modify the test to use it

Sorry, fixed!

chaodaiG

Thank you!

k8s-ci-robot · 2021-02-22T15:06:13Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chaodaiG, matthyx

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~~ [chaodaiG]

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

chaodaiG · 2021-02-23T19:19:40Z

/unhold

cjwagner · 2021-02-26T19:33:39Z

This is handy, thanks Matthias!

matthyx · 2021-02-26T20:03:10Z

This is handy, thanks Matthias!

Now I will rework my never ending contribution for config trees... I'm almost sure by the time I get it merged go will support generics 😂

fejta · 2021-02-26T22:26:47Z

What documentation am I supposed to follow to do this migration?

fejta · 2021-02-26T22:27:07Z

This is somewhat unhelpful:

msg: "plugins declaration uses a deprecated config style, please migrate it"
file: "prow/plugins/config.go:843"

fejta · 2021-02-26T22:31:11Z

Also can you please update this logging to only say this once per process and/or once per hour? Right now this is the only thing visible in our logs.

k8s-ci-robot requested a review from chaodaiG February 2, 2021 15:30

matthyx force-pushed the 20631 branch 2 times, most recently from 1d4f82d to 053c16c Compare February 4, 2021 10:57

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 Feb 4, 2021

matthyx force-pushed the 20631 branch from 053c16c to 8e780b9 Compare February 5, 2021 10:34

k8s-ci-robot added area/prow/deck Issues or PRs related to prow's deck component area/prow/hook Issues or PRs related to prow's hook component labels Feb 5, 2021

matthyx force-pushed the 20631 branch from 8e780b9 to 0a0e3f4 Compare February 5, 2021 16:48

matthyx force-pushed the 20631 branch 2 times, most recently from 62c1c75 to 263dae3 Compare February 17, 2021 08:21

matthyx commented Feb 17, 2021

View reviewed changes

matthyx changed the title ~~WIP - Add support of excluded_repos for plugins~~ Add support of excluded_repos for plugins Feb 17, 2021

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 17, 2021

matthyx force-pushed the 20631 branch from 263dae3 to 007f9d2 Compare February 17, 2021 08:57

chaodaiG requested changes Feb 17, 2021

View reviewed changes

k8s-ci-robot assigned chaodaiG Feb 17, 2021

matthyx force-pushed the 20631 branch from 007f9d2 to 5f9d670 Compare February 18, 2021 14:30

matthyx force-pushed the 20631 branch from d30a06a to cd373ab Compare February 19, 2021 15:45

chaodaiG approved these changes Feb 19, 2021

View reviewed changes

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Feb 19, 2021

matthyx force-pushed the 20631 branch from cd373ab to 7e4188f Compare February 19, 2021 15:52

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

chaodaiG approved these changes Feb 19, 2021

View reviewed changes

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

chaodaiG reviewed Feb 19, 2021

View reviewed changes

Add support of excluded_repos for plugins

bd70e36

matthyx force-pushed the 20631 branch from 7e4188f to bd70e36 Compare February 22, 2021 06:41

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

chaodaiG approved these changes Feb 22, 2021

View reviewed changes

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

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 23, 2021

k8s-ci-robot merged commit ad856c1 into kubernetes:master Feb 23, 2021

k8s-ci-robot added this to the v1.21 milestone Feb 23, 2021

spiffxp mentioned this pull request Feb 26, 2021

prow: avoid retriggering presubmits on default remote branch rename #20829

Closed

fejta mentioned this pull request Feb 26, 2021

plugins.yaml uses old format #21057

Closed

matthyx deleted the 20631 branch April 15, 2021 07:48

cjwagner mentioned this pull request Sep 11, 2024

blunderbuss: allow request_count to be set to '0' effectively disabling functionality kubernetes-sigs/prow#273

Closed

Add support of excluded_repos for plugins #20707

Add support of excluded_repos for plugins #20707

Conversation

matthyx commented Feb 2, 2021

matthyx commented Feb 2, 2021

matthyx commented Feb 2, 2021

chaodaiG commented Feb 2, 2021

chaodaiG commented Feb 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthyx commented Feb 17, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chaodaiG Feb 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chaodaiG commented Feb 18, 2021

chaodaiG left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthyx commented Feb 19, 2021

chaodaiG left a comment

Choose a reason for hiding this comment

chaodaiG left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chaodaiG left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Feb 22, 2021

chaodaiG commented Feb 23, 2021

cjwagner commented Feb 26, 2021

matthyx commented Feb 26, 2021

fejta commented Feb 26, 2021

fejta commented Feb 26, 2021

fejta commented Feb 26, 2021

chaodaiG Feb 17, 2021 •

edited

Loading