Add org and org/repo sharding for Blunderbuss

[dhaiducek]

Currently we have a bot user whose PRs should be ignored by Blunderbuss, a feature which was implemented in this PR:

• Add IgnoreAuthors option to Blunderbuss plugin #27136

However, we can't yet leverage the feature because we need to be able to specify a Blunderbuss configuration at our repo level since the Prow administrators don't want to have the same configuration globally.

[k8s-ci-robot]

Hi @dhaiducek. 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

[dhaiducek]

/retest

[dhaiducek]

Any updates here?

[jmguzik]

@dhaiducek I see you waited long for the review, but if you are still willing to go through the process and still interested in the PR we can speed things up. I did some entry review of your PR and have some questions/comments.

[jmguzik]

Is this the only possible test case? What about different config options?

[dhaiducek]

I can add some more test cases to make sure things are wired up. 🙂

I was wondering about that also though, and I'll have to double check this, but it looks like in general sharding replaces the configuration in its entirety rather than checking each value and providing overrides. Am I looking at that correctly?

[jmguzik]

Yes, that is my understanding as well. More test cases would be nice.

[jmguzik]

/retest

[dhaiducek]

Sorry for the churn here! Here's the TLDR:

• I refactored a bit (primarily pulled the BlunderbussFor function up into the Handle*Event functions so that the diff in blunderbuss.go would be less)
• Wrote tests for MergeFrom (which raised some merging logic bugs that were addressed 🪲) and additional tests in blunderbuss_test.go

I think this should be ready for another review now.

[k8s-ci-robot]

@dhaiducek: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-test-infra-verify-cri-o	961d51a	link	true	/test pull-test-infra-verify-cri-o

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

[jmguzik]

@droslean could you take a look? It would be nice to enable it in our prow.

[jmguzik]

/cc @droslean

[petr-muller]

I'll happily approve this! I trust Jakub and I have only skimmed the code; I have one nit about the serialization.

One question: does the implementation follow how the other plugins configs are sharded?

@jmguzik IIRC in the past, when we implemented the other sharding bits it bit us through the "load and save Prow config" job that we had in openshift - it loaded the unsharded config and dumped the sharded one, so this needed manual attention once we deployed the Prow with the changes. Just something to keep in mind.

[jmguzik]

@jmguzik IIRC in the past, when we implemented the other sharding bits it bit us through the "load and save Prow config" job that we had in openshift - it loaded the unsharded config and dumped the sharded one, so this needed manual attention once we deployed the Prow with the changes. Just something to keep in mind.

Thanks @petr-muller I suspected something like this can happen.

[dhaiducek]

Sorry for the delay here--I got caught up with some other work. Thanks for the reviews, @petr-muller and @jmguzik! I've updated the key to snake case.
It sounds like the other comments around sharding are a gotcha around Openshift tooling, or is there something that should be adjusted here?

[dhaiducek]

@petr-muller I neglected to answer the sharding alignment question. The answer is "sort of". There isn't an entirely consistent way that configurations are sharded, but I made an attempt to choose and follow other existing conventions.

Most notable, this implementation uses a flattened map, OrgsRepos containing org and org/repo keys pointing to each configuration, which I liked for simplicity:

orgs_repos:
  "org": <configuration>
  "org/repo": <configuration>

Whereas some other sharded configurations (like Tide.context_options, Branch Protection, and Bugzilla) chose to use an Orgs map containing a nested Repos map:

orgs:
  "org":
    <configuration>
    repos:
      "repo": <configuration>

The downside to this implementation I'm realizing now is that it doesn't readily allow for org-specific options in the future like the nested repos map does, but for something like pull request reviews where it's inherently a repository operation, I'm not sure there's a need for org-specific options. But I can adjust it if that should be a consideration.

[jmguzik]

@dhaiducek sorry for the late response. While waiting for a comment from @petr-muller, my personal opinion goes toward making it similar to other plugins. So I would vote for option:

orgs:
  "org":
    <configuration>
    repos:
      "repo": <configuration>

[dhaiducek]

For consideration, I've updated the struct to use nested orgs and nested repos as discussed in a new commit. It uses a pointer, *BlunderbussConfig, at the organization level to readily determine whether a config was defined there (otherwise, it wouldn't be possible to tell the difference between a configuration with defaults and an empty configuration).

[petr-muller]

LGTM

Thanks for the effort invested into this, and sorry for it to take this long.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dhaiducek, jmguzik, 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/cmd/deck/OWNERS [petr-muller]
• prow/plugins/OWNERS [petr-muller]

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