Add Orgs sharding for Blunderbuss by dhaiducek · Pull Request #237 · kubernetes-sigs/prow

dhaiducek · 2024-08-16T20:30:39Z

Using a pointer lets sharding determine whether a config had been provided so they can be properly compared. If one is not provided, this also instantiates one in setDefaults(), which is called following the sharding logic.

Migrated from:

Fix sharding by using a pointer for global Blunderbuss config kubernetes/test-infra#29405

k8s-ci-robot · 2024-08-16T20:30:49Z

Hi @dhaiducek. Thanks for your PR.

I'm waiting for a kubernetes-sigs 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.

Details

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-sigs/prow repository.

netlify · 2024-08-16T20:30:57Z

✅ Deploy Preview for k8s-prow ready!

Name	Link
🔨 Latest commit	`9556627`
🔍 Latest deploy log	https://app.netlify.com/projects/k8s-prow/deploys/6862ff951b74c20008cca472
😎 Deploy Preview	https://deploy-preview-237--k8s-prow.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

jmguzik · 2024-08-17T08:02:04Z

/ok-to-test

petr-muller · 2024-10-23T12:55:36Z

/cc

k8s-triage-robot · 2025-01-21T13:22:11Z

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle stale
Close this PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

dhaiducek · 2025-01-21T13:27:59Z

/remove-lifecycle stale

jmguzik

@dhaiducek Are you still interested and willing to push it further?
I feel really bad about this one as I was involved here kubernetes/test-infra#28169 and I think we even pushed it, but missed detail about pointer. Correct me if I am wrong.

This code was reviewed so generally from my POV there is nothing major to comment. This change is huge though and it can affect again prow installations, including openshift one. I am just loudly thinking how we should approach the testing regarding the topic.

It would be ideal to test it in some kind of puppy environment, but it's not straightforward to setup, but maybe worth a try.

dhaiducek · 2025-01-30T20:52:37Z

@dhaiducek Are you still interested and willing to push it further?

Hey @jmguzik! Yes, I'm willing to dive back into it--it's still relevant for us. No hard feelings--I got pretty gun-shy myself when it merged and broke things and had to be reverted, and I know it's a massive change. But yes, as I recall it broke last time because the implementation didn't use a pointer for the global config and didn't correctly identify when another global config wasn't specified.

I wouldn't know the first thing about setting up a sandbox Prow env, but I can try or let me know if there's anything I can do to help! Before, if I'm looking at the thread correctly, it failed during the rehearsal CI and never made it into production: https://redhat-internal.slack.com/archives/CBN38N3MW/p1683036380568929

jmguzik · 2025-02-04T19:14:08Z

I'll test that on puppy env during next shift week to see if the current impl behaves 😉

petr-muller · 2025-03-18T22:59:28Z

/area plugins

k8s-ci-robot · 2025-03-19T14:34:17Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: dhaiducek
Once this PR has been reviewed and has the lgtm label, please assign michelle192837, petr-muller for approval. For more information see the Code Review Process.

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

Details

Needs approval from an approver in each of these files:

cmd/deck/OWNERS
pkg/OWNERS

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

jmguzik

I would like to get this closer to merging this week. Left some comments/suggestions.

jmguzik · 2025-03-25T08:12:13Z

+	}
+
+	// Return base config
+	return *c.Blunderbuss.BlunderbussConfig


Suggested change

return *c.Blunderbuss.BlunderbussConfig

if c.Blunderbuss.BlunderbussConfig != nil {

return *c.Blunderbuss.BlunderbussConfig

}

return BlunderbussConfig{}

makes sense?

It could make sense, but I'm not sure it's necessary since the nil case is handled here in setDefaults():
https://github.com/kubernetes-sigs/prow/pull/237/files#diff-19905b632cb1213462d0e01d336d4f49504923b1365fceca80506f7f9a755a61R1129-R1132

jmguzik · 2025-03-25T08:24:02Z

+			b.BlunderbussConfig = other.BlunderbussConfig
+		} else if !reflect.DeepEqual(b.BlunderbussConfig, other.BlunderbussConfig) {
+			// Add error when both configs declare different global defaults
+			errs = append(errs, fmt.Errorf("global configurations for blunderbuss do not match"))


should that not be stopping error?

Not necessarily. By continuing on, it can aggregate all of the errors, not just the mismatch of the default values.

k8s-triage-robot · 2025-06-24T22:07:53Z

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle stale
Close this PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

dhaiducek

/remove-lifecycle stale

Sorry, I let this one flounder and get stale--I've left some responses to the review comments.

dhaiducek · 2025-06-30T20:32:45Z

+			b.BlunderbussConfig = other.BlunderbussConfig
+		} else if !reflect.DeepEqual(b.BlunderbussConfig, other.BlunderbussConfig) {
+			// Add error when both configs declare different global defaults
+			errs = append(errs, fmt.Errorf("global configurations for blunderbuss do not match"))


Not necessarily. By continuing on, it can aggregate all of the errors, not just the mismatch of the default values.

dhaiducek · 2025-06-30T20:36:58Z

+	}
+
+	// Return base config
+	return *c.Blunderbuss.BlunderbussConfig


It could make sense, but I'm not sure it's necessary since the nil case is handled here in setDefaults():
https://github.com/kubernetes-sigs/prow/pull/237/files#diff-19905b632cb1213462d0e01d336d4f49504923b1365fceca80506f7f9a755a61R1129-R1132

Using a pointer lets sharding determine whether a config had been provided so they can be properly compared. If one is not provided, this also instantiates one in `setDefaults()`, which is called following the sharding logic. Signed-off-by: Dale Haiducek <19750917+dhaiducek@users.noreply.github.com>

k8s-triage-robot · 2025-09-28T21:38:56Z

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle stale
Close this PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

dhaiducek · 2025-09-28T23:52:26Z

/remove-lifecycle stale

k8s-triage-robot · 2025-12-27T23:59:45Z

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle stale
Close this PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

dhaiducek · 2026-01-05T20:00:05Z

/remove-lifecycle stale

k8s-triage-robot · 2026-04-05T20:30:19Z

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle stale
Close this PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

dhaiducek · 2026-04-06T01:11:23Z

/remove-lifecycle stale

petr-muller · 2026-04-06T12:19:37Z

We recently got a very good reason to get this merged, thanks for keeping it fresh! I started to review the PR and it looks good skimmed, I'll do some testing and look a bit closer and I hope we'll get this done soon.

dhaiducek · 2026-04-07T00:26:23Z

We recently got a very good reason to get this merged, thanks for keeping it fresh! I started to review the PR and it looks good skimmed, I'll do some testing and look a bit closer and I hope we'll get this done soon.

Sounds great, thanks @petr-muller! I'll keep an eye out for reviews/followups (and might have to look over the code to refresh myself on it too! 😅).

petr-muller

wow, it was criminal of us to not merge this sooner, it's pretty polished

my only question would be whether the handleStatus path should get the fragment config instead of the full one like the other handlers (inline)

I also noticed the validation is not done on fragments but that one is not that important as there's barely any validation there (although, in sharded configs validation becomes more important, so adding a shard validation that would do more checks would be a great followup)

petr-muller · 2026-04-19T21:09:59Z

 func handleStatusEvent(pc plugins.Agent, se github.StatusEvent) error {
 	if se.State == "" || se.Context == "" || se.Description == "" {
 		return fmt.Errorf("invalid status event delivered with empty state/context/description")
 	}

 	return handleStatus(
 		pc.GitHubClient,
 		pc.OwnersClient,
 		pc.Logger,
 		pc.PluginConfig.Blunderbuss,
 		se.SHA,
 		se.Context,
 		se.State,
 		se.Description,
 		&se.Repo,
 	)

 }

 func handleStatus(ghc githubClient, roc repoownersClient, log *logrus.Entry, config plugins.Blunderbuss, sha string, context string, state string, description string, repo *github.Repo) error {


I think handleStatus and handleStatusEvent need to weave the BlunderbussFor(...) result instead of the full config from PluginConfig, right?

petr-muller · 2026-04-19T21:20:30Z

 func validateBlunderbuss(b *Blunderbuss) error {
 	if b.ReviewerCount != nil && *b.ReviewerCount < 1 {
 		return fmt.Errorf("invalid request_count: %v (needs to be positive)", *b.ReviewerCount)
 	}
 	return nil
 }


I dont thick the check is that useful (among other things, other fields are not checked...) but this should descend into b.Orgs and check the layers below.

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 16, 2024

k8s-ci-robot requested review from MadhavJivrajani, Priyankasaggu11929, cjwagner and michelle192837 August 16, 2024 20:30

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 16, 2024

dhaiducek mentioned this pull request Aug 16, 2024

Fix sharding by using a pointer for global Blunderbuss config kubernetes/test-infra#29405

Closed

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 Aug 17, 2024

hashim21223445 mentioned this pull request Oct 9, 2024

Prow issue: ملفات Andoka Cloud والمستندات والقوالب والتعليمات لتطبيقات البرمجة #295

Closed

k8s-ci-robot requested a review from petr-muller October 23, 2024 12:55

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 21, 2025

k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 21, 2025

jmguzik reviewed Jan 30, 2025

View reviewed changes

k8s-ci-robot added area/plugins Issues or PRs related to prow's plugins for the hook component needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Mar 18, 2025

dhaiducek force-pushed the shard-blunderbuss-migrated branch from 569d41b to 38c961a Compare March 19, 2025 14:34

k8s-ci-robot added area/deck Issues or PRs related to prow's deck component and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Mar 19, 2025

jmguzik reviewed Mar 25, 2025

View reviewed changes

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 24, 2025

dhaiducek commented Jun 30, 2025

View reviewed changes

k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 30, 2025

dhaiducek force-pushed the shard-blunderbuss-migrated branch 3 times, most recently from 3a277ca to 2bbd15e Compare June 30, 2025 21:18

dhaiducek force-pushed the shard-blunderbuss-migrated branch from 2bbd15e to 9556627 Compare June 30, 2025 21:20

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 28, 2025

k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 28, 2025

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 27, 2025

k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 5, 2026

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 5, 2026

k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 6, 2026

petr-muller added a commit to petr-muller/prow that referenced this pull request Apr 9, 2026

kubernetes-sigs#237

45c0f46

petr-muller reviewed Apr 19, 2026

View reviewed changes

Conversation

dhaiducek commented Aug 16, 2024

Uh oh!

k8s-ci-robot commented Aug 16, 2024

Uh oh!

netlify Bot commented Aug 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for k8s-prow ready!

Uh oh!

jmguzik commented Aug 17, 2024

Uh oh!

petr-muller commented Oct 23, 2024

Uh oh!

k8s-triage-robot commented Jan 21, 2025

Uh oh!

dhaiducek commented Jan 21, 2025

Uh oh!

jmguzik left a comment

Choose a reason for hiding this comment

Uh oh!

dhaiducek commented Jan 30, 2025

Uh oh!

jmguzik commented Feb 4, 2025

Uh oh!

petr-muller commented Mar 18, 2025

Uh oh!

k8s-ci-robot commented Mar 19, 2025

Uh oh!

jmguzik left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

k8s-triage-robot commented Jun 24, 2025

Uh oh!

dhaiducek left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

k8s-triage-robot commented Sep 28, 2025

Uh oh!

dhaiducek commented Sep 28, 2025

Uh oh!

k8s-triage-robot commented Dec 27, 2025

Uh oh!

dhaiducek commented Jan 5, 2026

Uh oh!

k8s-triage-robot commented Apr 5, 2026

Uh oh!

dhaiducek commented Apr 6, 2026

Uh oh!

petr-muller commented Apr 6, 2026

Uh oh!

dhaiducek commented Apr 7, 2026

Uh oh!

petr-muller left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

netlify Bot commented Aug 16, 2024 •

edited

Loading