Add support for alternate default_decoration_configs format that allows defaulting by cluster. #20656

cjwagner · 2021-01-29T05:42:57Z

I'm opening this as WIP because I still need to add some more tests and docs.

Design Doc: https://docs.google.com/document/d/1VtamQuaR-TFVud3NjCRUV8Nqtp4WmBK1na7fNNiYHXU/edit?resourcekey=0-VXcyCZ9kcLJxQnnrUx4p0Q

cjwagner · 2021-01-29T22:47:31Z

I don't know why I'm getting this error, I didn't touch anything related to this field AFAICT and this field was already unexported...

--- FAIL: TestGeneratePeriodics (0.00s)
panic: cannot handle unexported field at {[]config.Periodic}[0].interval:
	"k8s.io/test-infra/prow/config".Periodic
consider using a custom Comparer; if you control the implementation of type, you can also consider using an Exporter, AllowUnexported, or cmpopts.IgnoreUnexported [recovered]
	panic: cannot handle unexported field at {[]config.Periodic}[0].interval:
	"k8s.io/test-infra/prow/config".Periodic
consider using a custom Comparer; if you control the implementation of type, you can also consider using an Exporter, AllowUnexported, or cmpopts.IgnoreUnexported
goroutine 123 [running]:
testing.tRunner.func1.1(0x2302dc0, 0xc0001c9810)
	GOROOT/src/testing/testing.go:1057 +0x46a
testing.tRunner.func1(0xc00050e900)
	GOROOT/src/testing/testing.go:1060 +0x636
panic(0x2302dc0, 0xc0001c9810)
	GOROOT/src/runtime/panic.go:975 +0x3e9
github.com/google/go-cmp/cmp.validator.apply(0xc000453c20, 0x2476fc0, 0xc0001b4128, 0x1a6, 0x2476fc0, 0xc0000ecba8, 0x1a6)
	external/com_github_google_go_cmp/cmp/options.go:244 +0x70e
github.com/google/go-cmp/cmp.(*state).tryOptions(0xc000453c20, 0x2967f80, 0x2476fc0, 0x2476fc0, 0xc0001b4128, 0x1a6, 0x2476fc0, 0xc0000ecba8, 0x1a6, 0x0)
	external/com_github_google_go_cmp/cmp/compare.go:303 +0x15a
github.com/google/go-cmp/cmp.(*state).compareAny(0xc000453c20, 0x29417c0, 0xc000590700)
	external/com_github_google_go_cmp/cmp/compare.go:258 +0x434
github.com/google/go-cmp/cmp.(*state).compareStruct(0xc000453c20, 0x2967f80, 0x2482860, 0x2482860, 0xc0001b4000, 0x199, 0x2482860, 0xc0000eca80, 0x199)
	external/com_github_google_go_cmp/cmp/compare.go:427 +0x58e
github.com/google/go-cmp/cmp.(*state).compareAny(0xc000453c20, 0x2941780, 0xc00007c900)
	external/com_github_google_go_cmp/cmp/compare.go:286 +0x17a8
github.com/google/go-cmp/cmp.(*state).statelessCompare(0xc000453c20, 0x2941780, 0xc00007c900, 0x2967f80, 0x2967f80)
	external/com_github_google_go_cmp/cmp/compare.go:229 +0xfc
github.com/google/go-cmp/cmp.(*state).compareSlice.func2(0x0, 0x0, 0x0, 0x353a1c0)
	external/com_github_google_go_cmp/cmp/compare.go:493 +0x105
github.com/google/go-cmp/cmp/internal/diff.Difference(0x5, 0x5, 0xc000597210, 0x5, 0x5, 0xc000041880)
	external/com_github_google_go_cmp/cmp/internal/diff/diff.go:241 +0x2b4
github.com/google/go-cmp/cmp.(*state).compareSlice(0xc000453c20, 0x2967f80, 0x22c8f20, 0x22c8f20, 0xc00000e340, 0x97, 0x22c8f20, 0xc00000e360, 0x97)
	external/com_github_google_go_cmp/cmp/compare.go:492 +0x852
github.com/google/go-cmp/cmp.(*state).compareAny(0xc000453c20, 0x2932480, 0xc0002a5ec0)
	external/com_github_google_go_cmp/cmp/compare.go:288 +0x117f
github.com/google/go-cmp/cmp.Diff(0x22c8f20, 0xc00000e340, 0x22c8f20, 0xc00000e360, 0x0, 0x0, 0x0, 0x6, 0x0)
	external/com_github_google_go_cmp/cmp/compare.go:119 +0x105
k8s.io/apimachinery/pkg/util/diff.legacyDiff(...)
	external/io_k8s_apimachinery/pkg/util/diff/diff.go:51
k8s.io/apimachinery/pkg/util/diff.ObjectDiff(...)
	external/io_k8s_apimachinery/pkg/util/diff/diff.go:58
k8s.io/test-infra/releng/config-forker.TestGeneratePeriodics(0xc00050e900)
	releng/config-forker/main_test.go:704 +0x19b2
testing.tRunner(0xc00050e900, 0x2657c78)
	GOROOT/src/testing/testing.go:1108 +0x203
created by testing.(*T).Run
	GOROOT/src/testing/testing.go:1159 +0x797

cjwagner · 2021-01-29T23:11:32Z

Looks like the diff handling was already broken in that test case, but only exposed as a problem if the test case fails. I fixed that.

cjwagner · 2021-01-30T00:06:48Z

More apparently unrelated errors for boilerplate in files I didn't touch...

2 files have incorrect boilerplate headers:
prow/test/integration/test/crier_test.go
prow/test/integration/test/overall_test.go

It is unclear to me why this started failing now, but didn't fail on the PR that introduced the boilerplate: #20521

spiffxp · 2021-01-30T17:16:04Z

/cc

chaodaiG

Generally looks good to me. Will come back again when there are more unit tests and documentation

chaodaiG · 2021-02-09T16:19:27Z

prow/config/config.go

+	// Repo matches against the "org/repo" that the presubmit or postsubmit is
+	// associated with. If the job is a periodic, extra_refs[0] is used. If the
+	// job is a periodic without extra_refs, the empty string will be used.
+	Repo *regexp.Regexp `json:"-"`


Is there a particular reason must use regex instead of existing * style? Ask user to learn something new is fine imo as long as it's well communicated. I'm interested in the reasoning behind this change.

I don't think there is an existing * (glob? wildcard?) matching style. The global config used key *, but that was just a special case literal match.
I think regexp matching is overkill here, but I figured it would be most appropriate for consistency with other Prow config fields. run_if_changed, branches, exclude_branches, trigger, etc. all use regular expressions rather than * matching.

I also considered naming this field OrgRepo and just checking for string equality with the org or org/repo. This is consistent with other parts of Prow config, but I thought it more important that all the matching/filtering fields in this struct use a consistent matching system. I think it could be confusing (especially if we add more filter fields) if some things are matching literally (possibly against multiple fields), some are using * matching, and some are using regexp.
WDYT?

I don't have a strong preference on repo field, I think either special case *, glob, or regex works for me, my major concern is the patterns for org/repo in other parts of prow are not using regex (they each have their own style, which I agree that it would be nice to be consistent across the board). And for cluster filed, I think the only reason using regex instead of literal is because of global default right? That would be fine with me as well.

And for cluster filed, I think the only reason using regex instead of literal is because of global default right?

I was thinking it might be nice to reduce config size by using the cluster names to apply config across common clusters. e.g. All clusters with -trusted suffix or all clusters containing istio. This isn't that important though.

/assign @alvaroaleman
WDYT Alvaro?

Hm, today we are fairly consistent for the orgRepo field by having the *, org or org/repo as choices, samples:

test-infra/prow/config/tide.go

Line 114 in b0edc0a

PRStatusBaseURLs map[string]string `json:"pr_status_base_urls,omitempty"`

test-infra/prow/config/config.go

Line 495 in b0edc0a

JobURLPrefixConfig map[string]string `json:"job_url_prefix_config,omitempty"`

test-infra/prow/config/config.go

Lines 918 to 919 in b0edc0a

// Use `org/repo`, `org` or `*` as key and an `SlackReporter` struct as value.

type SlackReporterConfigs map[string]SlackReporter

Off of the top of my head I would also think that allowing regexes there is mostly prone to cause issues (someone creates a new repo that happens to match some regex it wasn't intended to match). run_if_changed, branches, eclude_branches are IMHO not great counterexamples, because they need to provide capabilities to match against anything (more or less), whereas the format here is fairly clear and limited. trigger is actually documented to match org or org/repo, on a five second glance I don't see anything regex in there:

test-infra/prow/plugins/config.go

Lines 363 to 364 in b0edc0a

// Repos is either of the form org/repos or just org.

Repos []string `json:"repos,omitempty"`

Ok that all makes good sense to me.
What are your thoughts on the cluster field then? I had hoped to use a consistent matching system, but the more I think about it, the more I'm thinking we should just have cluster be a literal string match, at least initially. That is simpler and likely sufficient for existing use cases. With this new config slice pattern we can easily add more matching fields like repo_re or cluster_re down the road if we find a need for them.

I don't really have an opinion regarding regex or not for cluster. It makes IMHO more sense than using it for repo, because for clusters you probably could have some kind of naming scheme that's based on trust domain but at the end of the day it boils down to what you need there and you know that better than me

Ok I'm going to go with literal matching then, I think that will be sufficient for now. I like the idea of expanding functionality with something like cluster_re if we find that we need alternative matching systems for any of these fields later on.

releng/config-forker/main.go

cjwagner

Will come back again when there are more unit tests and documentation

Thanks, I ran into some more unrelated issues last night, but I have the majority of the tests done now. I'll notify when this is ready for another review.

releng/config-forker/main.go

cjwagner · 2021-02-09T19:03:41Z

prow/config/config.go

+	// Repo matches against the "org/repo" that the presubmit or postsubmit is
+	// associated with. If the job is a periodic, extra_refs[0] is used. If the
+	// job is a periodic without extra_refs, the empty string will be used.
+	Repo *regexp.Regexp `json:"-"`


I don't think there is an existing * (glob? wildcard?) matching style. The global config used key *, but that was just a special case literal match.
I think regexp matching is overkill here, but I figured it would be most appropriate for consistency with other Prow config fields. run_if_changed, branches, exclude_branches, trigger, etc. all use regular expressions rather than * matching.

I also considered naming this field OrgRepo and just checking for string equality with the org or org/repo. This is consistent with other parts of Prow config, but I thought it more important that all the matching/filtering fields in this struct use a consistent matching system. I think it could be confusing (especially if we add more filter fields) if some things are matching literally (possibly against multiple fields), some are using * matching, and some are using regexp.
WDYT?

cjwagner · 2021-03-04T19:57:30Z

PTAL, I think the way I implemented this preserves the reserialization behavior while still using 2 separate fields to make the generated docs more clear.
Only the newest commit contains substantive code changes since the last review, other commits were just rebased.

alvaroaleman · 2021-03-05T19:02:51Z

Sorry, a bit underwater atm, will look early next week

alvaroaleman · 2021-03-08T15:21:33Z

Yeah, the way it's implemented now looks good :) There are still a few outstanding comments from a previous review, though

…ws defaulting by cluster.

…son.RawMessage.

cjwagner · 2021-03-13T00:27:18Z

There are still a few outstanding comments from a previous review, though

My apologies, I forgot to implement those changes. I believe I've addressed everything now.

cjwagner · 2021-03-16T21:47:40Z

@chaodaiG @alvaroaleman PTAL. Changes since the last review are in their own commit for convenience.

alvaroaleman

/lgtm

Looks good, thanks for all the work! Could you squash it though (or apply the tide squash label)?

alvaroaleman · 2021-03-17T13:45:13Z

/hold

k8s-ci-robot · 2021-03-17T15:09:38Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alvaroaleman, chaodaiG, cjwagner

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:

~~config/OWNERS~~ [chaodaiG,cjwagner]
~~prow/OWNERS~~ [alvaroaleman,chaodaiG,cjwagner]
~~releng/OWNERS~~ [chaodaiG,cjwagner]
~~testgrid/OWNERS~~ [chaodaiG,cjwagner]

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

cjwagner · 2021-03-17T18:21:30Z

/label tide/merge-method-squash
/hold cancel

…on_config_entries Catching up with 1acac76 (Add support for alternate default_decoration_configs format that allows defaulting by cluster, 2021-03-17, kubernetes#20656), which transitioned the content from a map to an entry.

…on_config_entries Catching up with 1acac76 (Add support for alternate default_decoration_configs format that allows defaulting by cluster, 2021-03-17, kubernetes#20656), which transitioned the content from a map to an entry. Also some similar edits in other places, where I haven't drilled into the history as closely.

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 29, 2021

k8s-ci-robot requested review from chaodaiG and clarketm January 29, 2021 05:43

cjwagner changed the title ~~Add support for alternate default_decoration_configs format that allows defaulting by cluster.~~ [WIP] Add support for alternate default_decoration_configs format that allows defaulting by cluster. Jan 29, 2021

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

cjwagner force-pushed the decoration-defaulting branch from 6973265 to 0b5e505 Compare January 29, 2021 20:38

k8s-ci-robot added the area/testgrid label Jan 29, 2021

cjwagner force-pushed the decoration-defaulting branch 2 times, most recently from 48a6efe to d0150b6 Compare January 29, 2021 22:23

k8s-ci-robot added area/config Issues or PRs related to code in /config area/release-eng Issues or PRs related to the Release Engineering subproject sig/release Categorizes an issue or PR as relevant to SIG Release. labels Jan 29, 2021

cjwagner force-pushed the decoration-defaulting branch from d0150b6 to 5be9725 Compare January 29, 2021 23:11

k8s-ci-robot requested a review from spiffxp January 30, 2021 17:16

chaodaiG mentioned this pull request Feb 5, 2021

Approvers: Add AutoApproveUnownedSubfolders option #20743

Merged

chaodaiG reviewed Feb 9, 2021

View reviewed changes

cjwagner commented Feb 9, 2021

View reviewed changes

cjwagner force-pushed the decoration-defaulting branch from 49de076 to 7db6b2a Compare February 10, 2021 02:55

k8s-ci-robot added the area/prow/plank Issues or PRs related to prow's plank component label Feb 10, 2021

cjwagner added 5 commits March 12, 2021 15:11

Add support for alternate default_decoration_configs format that allo…

5564134

…ws defaulting by cluster.

Add unit tests for new DefaultDecorationConfigs format.

7a7bb68

Update documentation about default_decoration_configs.

784fed1

Expose fields for both default_decoration formats rather than using j…

d06a842

…son.RawMessage.

Respond to review. Namely replace regexp filters.

0fc8369

cjwagner force-pushed the decoration-defaulting branch from caf55c3 to 0fc8369 Compare March 13, 2021 00:25

alvaroaleman approved these changes Mar 17, 2021

View reviewed changes

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

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

chaodaiG approved these changes Mar 17, 2021

View reviewed changes

k8s-ci-robot assigned chaodaiG Mar 17, 2021

k8s-ci-robot added tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Mar 17, 2021

k8s-ci-robot merged commit 1acac76 into kubernetes:master Mar 17, 2021

k8s-ci-robot added this to the v1.21 milestone Mar 17, 2021

cjwagner deleted the decoration-defaulting branch May 4, 2021 00:48

wking mentioned this pull request Jun 23, 2021

prow/plank/README: Fix default_decoration_configs -> default_decoration_config_entries #22679

Merged

cjwagner mentioned this pull request Aug 2, 2021

cluster and repo in plank.default_decoration_config_entries[].config don't support regexes #23096

Closed

palnabarun mentioned this pull request Nov 15, 2021

Add merge restrictions for Kubernetes 1.23 Code Freeze palnabarun/test-infra#1

Closed

chaodaiG mentioned this pull request May 23, 2022

Rerun auth config does not work well for periodic Prow jobs #25570

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for alternate default_decoration_configs format that allows defaulting by cluster. #20656

Add support for alternate default_decoration_configs format that allows defaulting by cluster. #20656

cjwagner commented Jan 29, 2021 •

edited

Loading

cjwagner commented Jan 29, 2021

cjwagner commented Jan 29, 2021

cjwagner commented Jan 30, 2021

spiffxp commented Jan 30, 2021

chaodaiG left a comment

chaodaiG Feb 9, 2021

cjwagner Feb 9, 2021

chaodaiG Feb 11, 2021

cjwagner Feb 11, 2021

alvaroaleman Feb 12, 2021

cjwagner Feb 20, 2021

alvaroaleman Feb 22, 2021

cjwagner Feb 24, 2021

cjwagner left a comment

cjwagner Feb 9, 2021

cjwagner commented Mar 4, 2021

alvaroaleman commented Mar 5, 2021

alvaroaleman commented Mar 8, 2021

cjwagner commented Mar 13, 2021

cjwagner commented Mar 16, 2021

alvaroaleman left a comment

alvaroaleman commented Mar 17, 2021

k8s-ci-robot commented Mar 17, 2021

cjwagner commented Mar 17, 2021

	// Use `org/repo`, `org` or `*` as key and an `SlackReporter` struct as value.
	type SlackReporterConfigs map[string]SlackReporter

	// Repos is either of the form org/repos or just org.
	Repos []string `json:"repos,omitempty"`

Add support for alternate default_decoration_configs format that allows defaulting by cluster. #20656

Add support for alternate default_decoration_configs format that allows defaulting by cluster. #20656

Conversation

cjwagner commented Jan 29, 2021 • edited Loading

cjwagner commented Jan 29, 2021

cjwagner commented Jan 29, 2021

cjwagner commented Jan 30, 2021

spiffxp commented Jan 30, 2021

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

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

cjwagner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjwagner commented Mar 4, 2021

alvaroaleman commented Mar 5, 2021

alvaroaleman commented Mar 8, 2021

cjwagner commented Mar 13, 2021

cjwagner commented Mar 16, 2021

alvaroaleman left a comment

Choose a reason for hiding this comment

alvaroaleman commented Mar 17, 2021

k8s-ci-robot commented Mar 17, 2021

cjwagner commented Mar 17, 2021

cjwagner commented Jan 29, 2021 •

edited

Loading