From 4d48676eb2102369c0a984f96d256ed9306933e9 Mon Sep 17 00:00:00 2001 From: tstollin Date: Wed, 15 Jul 2026 09:04:57 +0200 Subject: [PATCH 1/2] feat: support required_reviewers in pull_request rules note: this feature is currently in beta and subject to change --- Makefile | 6 +- .../api/v1alpha1/pullrequestreviewerentity.go | 62 ++++ .../api/v1alpha1/pullrequestrule.go | 16 + .../v1alpha1/requiredpullrequestreviewer.go | 68 +++++ .../applyconfiguration/internal/internal.go | 36 +++ api/v1alpha1/applyconfiguration/utils.go | 4 + api/v1alpha1/rulesetpreset_types.go | 46 +++ api/v1alpha1/zz_generated.deepcopy.go | 53 ++++ cmd/main.go | 12 +- .../github.interhyp.de_rulesetpresets.yaml | 60 ++++ docs/crds.md | 39 +++ internal/mapper/github_ruleset_mapper.go | 69 +++++ internal/mapper/github_ruleset_mapper_test.go | 273 ++++++++++++++++++ internal/reconciler/shared.go | 33 +++ internal/reconciler/shared_test.go | 245 ++++++++++++++++ 15 files changed, 1016 insertions(+), 6 deletions(-) create mode 100644 api/v1alpha1/applyconfiguration/api/v1alpha1/pullrequestreviewerentity.go create mode 100644 api/v1alpha1/applyconfiguration/api/v1alpha1/requiredpullrequestreviewer.go diff --git a/Makefile b/Makefile index 2e59952..3a4c442 100644 --- a/Makefile +++ b/Makefile @@ -50,6 +50,7 @@ manifests: controller-gen ## Generate WebhookConfiguration, ClusterRole and Cust .PHONY: generate generate: controller-gen ## Generate code containing DeepCopy, DeepCopyInto, and DeepCopyObject method implementations. "$(CONTROLLER_GEN)" object:headerFile="hack/boilerplate.go.txt",year=$(YEAR) paths="./..." + "$(CONTROLLER_GEN)" applyconfiguration:headerFile="hack/boilerplate.go.txt" paths="./api/..." .PHONY: fmt fmt: ## Run go fmt against code. @@ -313,7 +314,7 @@ grafana: # $2 - package url which can be installed # $3 - specific version of package define go-install-tool -@[ -f "$(1)-$(3)" ] && [ "$$(readlink -- "$(1)" 2>/dev/null)" = "$(1)-$(3)" ] || { \ +@[ -f "$(1)-$(3)" ] || { \ set -e; \ package=$(2)@$(3) ;\ echo "Downloading $${package}" ;\ @@ -321,7 +322,8 @@ rm -f "$(1)" ;\ GOBIN="$(LOCALBIN)" go install $${package} ;\ mv "$(LOCALBIN)/$$(basename "$(1)")" "$(1)-$(3)" ;\ } ;\ -ln -sf "$$(realpath "$(1)-$(3)")" "$(1)" +[ -L "$(1)" ] && [ -e "$(1)" ] || \ +ln -sf "$$(basename "$(1)-$(3)")" "$(1)" endef define gomodver diff --git a/api/v1alpha1/applyconfiguration/api/v1alpha1/pullrequestreviewerentity.go b/api/v1alpha1/applyconfiguration/api/v1alpha1/pullrequestreviewerentity.go new file mode 100644 index 0000000..3cf6134 --- /dev/null +++ b/api/v1alpha1/applyconfiguration/api/v1alpha1/pullrequestreviewerentity.go @@ -0,0 +1,62 @@ +/* +Copyright 2025. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ +// Code generated by controller-gen. DO NOT EDIT. + +package v1alpha1 + +// PullRequestReviewerEntityApplyConfiguration represents a declarative configuration of the PullRequestReviewerEntity type for use +// with apply. +// +// PullRequestReviewerEntity defines who is required to review as part of a required reviewers pull request rule. +type PullRequestReviewerEntityApplyConfiguration struct { + // ID of the required reviewer. This field is exclusive with Slug. + ID *int64 `json:"id,omitempty"` + // Slug identifying the required reviewer. This field is exclusive with ID. + // Slug will be resolved to the id during reconciliation. + Slug *string `json:"slug,omitempty"` + // Type of the required reviewer. Currently only Teams are supported. + Type *string `json:"type,omitempty"` +} + +// PullRequestReviewerEntityApplyConfiguration constructs a declarative configuration of the PullRequestReviewerEntity type for use with +// apply. +func PullRequestReviewerEntity() *PullRequestReviewerEntityApplyConfiguration { + return &PullRequestReviewerEntityApplyConfiguration{} +} + +// WithID sets the ID field in the declarative configuration to the given value +// and returns the receiver, so that objects can be built by chaining "With" function invocations. +// If called multiple times, the ID field is set to the value of the last call. +func (b *PullRequestReviewerEntityApplyConfiguration) WithID(value int64) *PullRequestReviewerEntityApplyConfiguration { + b.ID = &value + return b +} + +// WithSlug sets the Slug field in the declarative configuration to the given value +// and returns the receiver, so that objects can be built by chaining "With" function invocations. +// If called multiple times, the Slug field is set to the value of the last call. +func (b *PullRequestReviewerEntityApplyConfiguration) WithSlug(value string) *PullRequestReviewerEntityApplyConfiguration { + b.Slug = &value + return b +} + +// WithType sets the Type field in the declarative configuration to the given value +// and returns the receiver, so that objects can be built by chaining "With" function invocations. +// If called multiple times, the Type field is set to the value of the last call. +func (b *PullRequestReviewerEntityApplyConfiguration) WithType(value string) *PullRequestReviewerEntityApplyConfiguration { + b.Type = &value + return b +} diff --git a/api/v1alpha1/applyconfiguration/api/v1alpha1/pullrequestrule.go b/api/v1alpha1/applyconfiguration/api/v1alpha1/pullrequestrule.go index 9a9dd66..88c3938 100644 --- a/api/v1alpha1/applyconfiguration/api/v1alpha1/pullrequestrule.go +++ b/api/v1alpha1/applyconfiguration/api/v1alpha1/pullrequestrule.go @@ -44,6 +44,9 @@ type PullRequestRuleApplyConfiguration struct { // RequiredReviewThreadResolution requires all review comment threads to be resolved before merging. // This ensures all feedback is addressed. RequiredReviewThreadResolution *bool `json:"requiredReviewThreadResolution,omitempty"` + // RequiredReviewers forces pull request approval by the configured reviewers + // on all pull requests changing files that match the configured file patterns. + RequiredReviewers []RequiredPullRequestReviewerApplyConfiguration `json:"requiredReviewers,omitempty"` } // PullRequestRuleApplyConfiguration constructs a declarative configuration of the PullRequestRule type for use with @@ -101,3 +104,16 @@ func (b *PullRequestRuleApplyConfiguration) WithRequiredReviewThreadResolution(v b.RequiredReviewThreadResolution = &value return b } + +// WithRequiredReviewers adds the given value to the RequiredReviewers field in the declarative configuration +// and returns the receiver, so that objects can be build by chaining "With" function invocations. +// If called multiple times, values provided by each call will be appended to the RequiredReviewers field. +func (b *PullRequestRuleApplyConfiguration) WithRequiredReviewers(values ...*RequiredPullRequestReviewerApplyConfiguration) *PullRequestRuleApplyConfiguration { + for i := range values { + if values[i] == nil { + panic("nil value passed to WithRequiredReviewers") + } + b.RequiredReviewers = append(b.RequiredReviewers, *values[i]) + } + return b +} diff --git a/api/v1alpha1/applyconfiguration/api/v1alpha1/requiredpullrequestreviewer.go b/api/v1alpha1/applyconfiguration/api/v1alpha1/requiredpullrequestreviewer.go new file mode 100644 index 0000000..6a3dd26 --- /dev/null +++ b/api/v1alpha1/applyconfiguration/api/v1alpha1/requiredpullrequestreviewer.go @@ -0,0 +1,68 @@ +/* +Copyright 2025. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ +// Code generated by controller-gen. DO NOT EDIT. + +package v1alpha1 + +// RequiredPullRequestReviewerApplyConfiguration represents a declarative configuration of the RequiredPullRequestReviewer type for use +// with apply. +// +// RequiredPullRequestReviewer is the configuration for a required review on all pull request changing files that match +// the configured file patterns. +type RequiredPullRequestReviewerApplyConfiguration struct { + // MinimumApprovals is the minimum number of approvals required before a matching pull request + // can be merged. If set to zero, the team will be added to the pull request but approval is optional. + // Requires a non-negative value. Defaults to 0. + MinimumApprovals *int `json:"minimumApprovals,omitempty"` + // FilePatterns are fnmatch syntax patterns that pull request changes are matched against. + // If a pull request changes any matching file it must be approved by the configured reviewers. + // Defaults to "*" representing all files. + FilePatterns []string `json:"filePatterns,omitempty"` + // Reviewer defines who is required to review. + Reviewer *PullRequestReviewerEntityApplyConfiguration `json:"reviewer,omitempty"` +} + +// RequiredPullRequestReviewerApplyConfiguration constructs a declarative configuration of the RequiredPullRequestReviewer type for use with +// apply. +func RequiredPullRequestReviewer() *RequiredPullRequestReviewerApplyConfiguration { + return &RequiredPullRequestReviewerApplyConfiguration{} +} + +// WithMinimumApprovals sets the MinimumApprovals field in the declarative configuration to the given value +// and returns the receiver, so that objects can be built by chaining "With" function invocations. +// If called multiple times, the MinimumApprovals field is set to the value of the last call. +func (b *RequiredPullRequestReviewerApplyConfiguration) WithMinimumApprovals(value int) *RequiredPullRequestReviewerApplyConfiguration { + b.MinimumApprovals = &value + return b +} + +// WithFilePatterns adds the given value to the FilePatterns field in the declarative configuration +// and returns the receiver, so that objects can be build by chaining "With" function invocations. +// If called multiple times, values provided by each call will be appended to the FilePatterns field. +func (b *RequiredPullRequestReviewerApplyConfiguration) WithFilePatterns(values ...string) *RequiredPullRequestReviewerApplyConfiguration { + for i := range values { + b.FilePatterns = append(b.FilePatterns, values[i]) + } + return b +} + +// WithReviewer sets the Reviewer field in the declarative configuration to the given value +// and returns the receiver, so that objects can be built by chaining "With" function invocations. +// If called multiple times, the Reviewer field is set to the value of the last call. +func (b *RequiredPullRequestReviewerApplyConfiguration) WithReviewer(value *PullRequestReviewerEntityApplyConfiguration) *RequiredPullRequestReviewerApplyConfiguration { + b.Reviewer = value + return b +} diff --git a/api/v1alpha1/applyconfiguration/internal/internal.go b/api/v1alpha1/applyconfiguration/internal/internal.go index ec0f8ec..29104fe 100644 --- a/api/v1alpha1/applyconfiguration/internal/internal.go +++ b/api/v1alpha1/applyconfiguration/internal/internal.go @@ -495,6 +495,18 @@ var schemaYAML = typed.YAMLObject(`types: - name: pattern type: scalar: string +- name: com.github.Interhyp.git-hubby.api.v1alpha1.PullRequestReviewerEntity + map: + fields: + - name: id + type: + scalar: numeric + - name: slug + type: + scalar: string + - name: type + type: + scalar: string - name: com.github.Interhyp.git-hubby.api.v1alpha1.PullRequestRule map: fields: @@ -523,6 +535,12 @@ var schemaYAML = typed.YAMLObject(`types: type: scalar: boolean default: false + - name: requiredReviewers + type: + list: + elementType: + namedType: com.github.Interhyp.git-hubby.api.v1alpha1.RequiredPullRequestReviewer + elementRelationship: atomic - name: com.github.Interhyp.git-hubby.api.v1alpha1.RefNameCondition map: fields: @@ -747,6 +765,24 @@ var schemaYAML = typed.YAMLObject(`types: map: elementType: namedType: com.github.Interhyp.git-hubby.api.v1alpha1.WebhookStatus +- name: com.github.Interhyp.git-hubby.api.v1alpha1.RequiredPullRequestReviewer + map: + fields: + - name: filePatterns + type: + list: + elementType: + scalar: string + elementRelationship: atomic + default: + - '*' + - name: minimumApprovals + type: + scalar: numeric + default: 0 + - name: reviewer + type: + namedType: com.github.Interhyp.git-hubby.api.v1alpha1.PullRequestReviewerEntity - name: com.github.Interhyp.git-hubby.api.v1alpha1.RequiredStatusChecks map: fields: diff --git a/api/v1alpha1/applyconfiguration/utils.go b/api/v1alpha1/applyconfiguration/utils.go index 547acd5..4659c69 100644 --- a/api/v1alpha1/applyconfiguration/utils.go +++ b/api/v1alpha1/applyconfiguration/utils.go @@ -85,6 +85,8 @@ func ForKind(kind schema.GroupVersionKind) interface{} { return &apiv1alpha1.OrgCustomPropertyDefaultValueApplyConfiguration{} case v1alpha1.SchemeGroupVersion.WithKind("PatternRule"): return &apiv1alpha1.PatternRuleApplyConfiguration{} + case v1alpha1.SchemeGroupVersion.WithKind("PullRequestReviewerEntity"): + return &apiv1alpha1.PullRequestReviewerEntityApplyConfiguration{} case v1alpha1.SchemeGroupVersion.WithKind("PullRequestRule"): return &apiv1alpha1.PullRequestRuleApplyConfiguration{} case v1alpha1.SchemeGroupVersion.WithKind("RefNameCondition"): @@ -101,6 +103,8 @@ func ForKind(kind schema.GroupVersionKind) interface{} { return &apiv1alpha1.RepositorySpecApplyConfiguration{} case v1alpha1.SchemeGroupVersion.WithKind("RepositoryStatus"): return &apiv1alpha1.RepositoryStatusApplyConfiguration{} + case v1alpha1.SchemeGroupVersion.WithKind("RequiredPullRequestReviewer"): + return &apiv1alpha1.RequiredPullRequestReviewerApplyConfiguration{} case v1alpha1.SchemeGroupVersion.WithKind("RequiredStatusChecks"): return &apiv1alpha1.RequiredStatusChecksApplyConfiguration{} case v1alpha1.SchemeGroupVersion.WithKind("RulesetBypassActor"): diff --git a/api/v1alpha1/rulesetpreset_types.go b/api/v1alpha1/rulesetpreset_types.go index 3841fde..30e902b 100644 --- a/api/v1alpha1/rulesetpreset_types.go +++ b/api/v1alpha1/rulesetpreset_types.go @@ -417,6 +417,52 @@ type PullRequestRule struct { // +optional // +default:value=false RequiredReviewThreadResolution *bool `json:"requiredReviewThreadResolution,omitempty"` + + // RequiredReviewers forces pull request approval by the configured reviewers + // on all pull requests changing files that match the configured file patterns. + // +optional + // +kubebuilder:validation:MaxItems=10 + RequiredReviewers []RequiredPullRequestReviewer `json:"requiredReviewers,omitempty"` +} + +// RequiredPullRequestReviewer is the configuration for a required review on all pull request changing files that match +// the configured file patterns. +type RequiredPullRequestReviewer struct { + // MinimumApprovals is the minimum number of approvals required before a matching pull request + // can be merged. If set to zero, the team will be added to the pull request but approval is optional. + // Requires a non-negative value. Defaults to 0. + // +optional + // +default:value=0 + // +kubebuilder:validation:Minimum=0 + MinimumApprovals int `json:"minimumApprovals,omitempty"` + + // FilePatterns are fnmatch syntax patterns that pull request changes are matched against. + // If a pull request changes any matching file it must be approved by the configured reviewers. + // Defaults to "*" representing all files. + // +optional + // +default:value=["*"] + // +kubebuilder:validation:MaxItems=10 + FilePatterns []string `json:"filePatterns,omitempty"` + + // Reviewer defines who is required to review. + // +kubebuilder:validation:Required + Reviewer PullRequestReviewerEntity `json:"reviewer"` +} + +// PullRequestReviewerEntity defines who is required to review as part of a required reviewers pull request rule. +// +kubebuilder:validation:ExactlyOneOf=id;slug +type PullRequestReviewerEntity struct { + // ID of the required reviewer. This field is exclusive with Slug. + // +optional + ID *int64 `json:"id,omitempty"` + // Slug identifying the required reviewer. This field is exclusive with ID. + // Slug will be resolved to the id during reconciliation. + // +optional + Slug *string `json:"slug,omitempty"` + // Type of the required reviewer. Currently only Teams are supported. + // +kubebuilder:validation:Required + // +kubebuilder:validation:Enum=Team + Type string `json:"type"` } // RequiredStatusChecks defines status check requirements that must pass before merging. diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go index a31cc14..5fd97b2 100644 --- a/api/v1alpha1/zz_generated.deepcopy.go +++ b/api/v1alpha1/zz_generated.deepcopy.go @@ -894,6 +894,31 @@ func (in *PatternRule) DeepCopy() *PatternRule { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *PullRequestReviewerEntity) DeepCopyInto(out *PullRequestReviewerEntity) { + *out = *in + if in.ID != nil { + in, out := &in.ID, &out.ID + *out = new(int64) + **out = **in + } + if in.Slug != nil { + in, out := &in.Slug, &out.Slug + *out = new(string) + **out = **in + } +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new PullRequestReviewerEntity. +func (in *PullRequestReviewerEntity) DeepCopy() *PullRequestReviewerEntity { + if in == nil { + return nil + } + out := new(PullRequestReviewerEntity) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *PullRequestRule) DeepCopyInto(out *PullRequestRule) { *out = *in @@ -922,6 +947,13 @@ func (in *PullRequestRule) DeepCopyInto(out *PullRequestRule) { *out = new(bool) **out = **in } + if in.RequiredReviewers != nil { + in, out := &in.RequiredReviewers, &out.RequiredReviewers + *out = make([]RequiredPullRequestReviewer, len(*in)) + for i := range *in { + (*in)[i].DeepCopyInto(&(*out)[i]) + } + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new PullRequestRule. @@ -1254,6 +1286,27 @@ func (in *RepositoryStatus) DeepCopy() *RepositoryStatus { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *RequiredPullRequestReviewer) DeepCopyInto(out *RequiredPullRequestReviewer) { + *out = *in + if in.FilePatterns != nil { + in, out := &in.FilePatterns, &out.FilePatterns + *out = make([]string, len(*in)) + copy(*out, *in) + } + in.Reviewer.DeepCopyInto(&out.Reviewer) +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new RequiredPullRequestReviewer. +func (in *RequiredPullRequestReviewer) DeepCopy() *RequiredPullRequestReviewer { + if in == nil { + return nil + } + out := new(RequiredPullRequestReviewer) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *RequiredStatusChecks) DeepCopyInto(out *RequiredStatusChecks) { *out = *in diff --git a/cmd/main.go b/cmd/main.go index 37556bf..d71ce4b 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -148,9 +148,9 @@ func buildLoggerOpts(flagOpts *zap.Options, format logFormat) []zap.Opts { switch format { case logFormatECS: - ecsConfigOpt := func(config *zapcore.EncoderConfig) { - if config != nil { - *config = ecszap.ECSCompatibleEncoderConfig(*config) + ecsConfigOpt := func(encConf *zapcore.EncoderConfig) { + if encConf != nil { + *encConf = ecszap.ECSCompatibleEncoderConfig(*encConf) } } logOpts = append(logOpts, @@ -358,7 +358,11 @@ func main() { } return &secret, nil } - clientManager, err := ghclient.NewGitHubCachingClientFactory(ghclient.DefaultClientConfig(), fetchSecret) + clientManager, err := ghclient.NewGitHubCachingClientFactory( + ghclient.DefaultClientConfig(), + fetchSecret, + appCredentialsSecretName, + ) if err != nil { setupLog.Error(err, "failed to create GitHub client factory") os.Exit(1) diff --git a/config/crd/bases/github.interhyp.de_rulesetpresets.yaml b/config/crd/bases/github.interhyp.de_rulesetpresets.yaml index c15b44c..20e4281 100644 --- a/config/crd/bases/github.interhyp.de_rulesetpresets.yaml +++ b/config/crd/bases/github.interhyp.de_rulesetpresets.yaml @@ -514,6 +514,66 @@ spec: RequiredReviewThreadResolution requires all review comment threads to be resolved before merging. This ensures all feedback is addressed. type: boolean + requiredReviewers: + description: |- + RequiredReviewers forces pull request approval by the configured reviewers + on all pull requests changing files that match the configured file patterns. + items: + description: |- + RequiredPullRequestReviewer is the configuration for a required review on all pull request changing files that match + the configured file patterns. + properties: + filePatterns: + default: + - '*' + description: |- + FilePatterns are fnmatch syntax patterns that pull request changes are matched against. + If a pull request changes any matching file it must be approved by the configured reviewers. + Defaults to "*" representing all files. + items: + type: string + maxItems: 10 + type: array + minimumApprovals: + default: 0 + description: |- + MinimumApprovals is the minimum number of approvals required before a matching pull request + can be merged. If set to zero, the team will be added to the pull request but approval is optional. + Requires a non-negative value. Defaults to 0. + minimum: 0 + type: integer + reviewer: + description: Reviewer defines who is required to review. + properties: + id: + description: ID of the required reviewer. This field + is exclusive with Slug. + format: int64 + type: integer + slug: + description: |- + Slug identifying the required reviewer. This field is exclusive with ID. + Slug will be resolved to the id during reconciliation. + type: string + type: + description: Type of the required reviewer. Currently + only Teams are supported. + enum: + - Team + type: string + required: + - type + type: object + x-kubernetes-validations: + - message: exactly one of the fields in [id slug] must + be set + rule: '[has(self.id),has(self.slug)].filter(x,x==true).size() + == 1' + required: + - reviewer + type: object + maxItems: 10 + type: array required: - allowedMergeMethods type: object diff --git a/docs/crds.md b/docs/crds.md index c4fe0f3..9ddf445 100644 --- a/docs/crds.md +++ b/docs/crds.md @@ -650,6 +650,25 @@ _Appears in:_ | `negate` _boolean_ | Negate inverts the pattern matching logic.
When true, the rule passes if the pattern does NOT match.
Example: Use with "contains" to prevent certain words in commit messages. | false | Optional: \{\}
| +#### PullRequestReviewerEntity + + + +PullRequestReviewerEntity defines who is required to review as part of a required reviewers pull request rule. + +_Validation:_ +- ExactlyOneOf: [id slug] + +_Appears in:_ +- [RequiredPullRequestReviewer](#requiredpullrequestreviewer) + +| Field | Description | Default | Validation | +| --- | --- | --- | --- | +| `id` _integer_ | ID of the required reviewer. This field is exclusive with Slug. | | Optional: \{\}
| +| `slug` _string_ | Slug identifying the required reviewer. This field is exclusive with ID.
Slug will be resolved to the id during reconciliation. | | Optional: \{\}
| +| `type` _string_ | Type of the required reviewer. Currently only Teams are supported. | | Enum: [Team]
Required: \{\}
| + + #### PullRequestRule @@ -670,6 +689,7 @@ _Appears in:_ | `requireLastPushApproval` _boolean_ | RequireLastPushApproval requires that the most recent push be approved.
This prevents merging if new commits are pushed after the last approval. | false | Optional: \{\}
| | `requiredApprovingReviewCount` _integer_ | RequiredApprovingReviewCount specifies the minimum number of approving reviews required.
Must be between 1 and 10. | | Maximum: 10
Minimum: 1
Optional: \{\}
| | `requiredReviewThreadResolution` _boolean_ | RequiredReviewThreadResolution requires all review comment threads to be resolved before merging.
This ensures all feedback is addressed. | false | Optional: \{\}
| +| `requiredReviewers` _[RequiredPullRequestReviewer](#requiredpullrequestreviewer) array_ | RequiredReviewers forces pull request approval by the configured reviewers
on all pull requests changing files that match the configured file patterns. | | MaxItems: 10
Optional: \{\}
| #### RefNameCondition @@ -855,6 +875,25 @@ _Appears in:_ | `observedSubResourceGenerations` _object (keys:string, values:integer)_ | ObservedSubResourceGenerations is a map of sub-resource names to their observed generations.
Keys are in the format "/".
SubResources are kubernetes resources that are referenced by this Repository and are not managed
by their own controllers like WebhookPresets, RuleSetPresets and the attached CodeSecurityConfiguration | | | +#### RequiredPullRequestReviewer + + + +RequiredPullRequestReviewer is the configuration for a required review on all pull request changing files that match +the configured file patterns. + + + +_Appears in:_ +- [PullRequestRule](#pullrequestrule) + +| Field | Description | Default | Validation | +| --- | --- | --- | --- | +| `minimumApprovals` _integer_ | MinimumApprovals is the minimum number of approvals required before a matching pull request
can be merged. If set to zero, the team will be added to the pull request but approval is optional.
Requires a non-negative value. Defaults to 0. | 0 | Minimum: 0
Optional: \{\}
| +| `filePatterns` _string array_ | FilePatterns are fnmatch syntax patterns that pull request changes are matched against.
If a pull request changes any matching file it must be approved by the configured reviewers.
Defaults to "*" representing all files. | [*] | MaxItems: 10
Optional: \{\}
| +| `reviewer` _[PullRequestReviewerEntity](#pullrequestreviewerentity)_ | Reviewer defines who is required to review. | | ExactlyOneOf: [id slug]
Required: \{\}
| + + #### RequiredStatusChecks diff --git a/internal/mapper/github_ruleset_mapper.go b/internal/mapper/github_ruleset_mapper.go index 3cd1ffd..fa05dc4 100644 --- a/internal/mapper/github_ruleset_mapper.go +++ b/internal/mapper/github_ruleset_mapper.go @@ -133,6 +133,7 @@ func mapRules(rules *githubv1alpha1.RulesetRules, ruleset *github.RepositoryRule RequireLastPushApproval: utils.WithDefault(rules.PullRequest.RequireLastPushApproval, false), RequiredApprovingReviewCount: rules.PullRequest.RequiredApprovingReviewCount, RequiredReviewThreadResolution: utils.WithDefault(rules.PullRequest.RequiredReviewThreadResolution, false), + RequiredReviewers: mapRequiredReviewers(rules.PullRequest.RequiredReviewers), } } @@ -185,6 +186,23 @@ func mapRules(rules *githubv1alpha1.RulesetRules, ruleset *github.RepositoryRule ruleset.Rules = githubRules } +func mapRequiredReviewers(reviewers []githubv1alpha1.RequiredPullRequestReviewer) []*github.RulesetRequiredReviewer { + result := make([]*github.RulesetRequiredReviewer, len(reviewers)) + for i, reviewer := range reviewers { + result[i] = &github.RulesetRequiredReviewer{ + MinimumApprovals: new(reviewer.MinimumApprovals), + FilePatterns: reviewer.FilePatterns, + Reviewer: &github.RulesetReviewer{ + // slugs have been resolved to id prior to mapping + ID: reviewer.Reviewer.ID, + // type conversion validated by enum validation of api field + Type: new(github.RulesetReviewerType(reviewer.Reviewer.Type)), + }, + } + } + return result +} + func mapMergeMethods(mergeMethods []string) []github.PullRequestMergeMethod { githubMergeMethods := make([]github.PullRequestMergeMethod, len(mergeMethods)) for i, method := range mergeMethods { @@ -592,6 +610,57 @@ func pullRequestRulesDiffer(presetPR *githubv1alpha1.PullRequestRule, githubPR * if utils.WithDefault(presetPR.RequiredReviewThreadResolution, false) != githubPR.RequiredReviewThreadResolution { return true } + if requiredReviewersDiffer(presetPR.RequiredReviewers, githubPR.RequiredReviewers) { + return true + } + + return false +} + +// requiredReviewersDiffer compares required reviewer lists between preset and GitHub pull request rule. +// Reviewers are matched by ID+type key; file patterns and minimum approvals are compared per entry. +func requiredReviewersDiffer(presetReviewers []githubv1alpha1.RequiredPullRequestReviewer, githubReviewers []*github.RulesetRequiredReviewer) bool { + if len(presetReviewers) != len(githubReviewers) { + return true + } + + type reviewerKey struct { + id int64 + typ string + } + + presetMap := make(map[reviewerKey]githubv1alpha1.RequiredPullRequestReviewer) + for _, r := range presetReviewers { + id := int64(0) + if r.Reviewer.ID != nil { + id = *r.Reviewer.ID + } + presetMap[reviewerKey{id: id, typ: r.Reviewer.Type}] = r + } + + for _, ghReviewer := range githubReviewers { + if ghReviewer.Reviewer == nil { + return true + } + id := int64(0) + if ghReviewer.Reviewer.ID != nil { + id = *ghReviewer.Reviewer.ID + } + typ := "" + if ghReviewer.Reviewer.Type != nil { + typ = string(*ghReviewer.Reviewer.Type) + } + presetReviewer, exists := presetMap[reviewerKey{id: id, typ: typ}] + if !exists { + return true + } + if presetReviewer.MinimumApprovals != utils.WithDefault(ghReviewer.MinimumApprovals, 0) { + return true + } + if !stringSlicesEqual(presetReviewer.FilePatterns, ghReviewer.FilePatterns) { + return true + } + } return false } diff --git a/internal/mapper/github_ruleset_mapper_test.go b/internal/mapper/github_ruleset_mapper_test.go index 3aa3c91..22197e6 100644 --- a/internal/mapper/github_ruleset_mapper_test.go +++ b/internal/mapper/github_ruleset_mapper_test.go @@ -1877,6 +1877,279 @@ var _ = Describe("GitHub Ruleset Mapper", func() { }) }) + Context("RulesetPresetToGithubRuleset with required reviewers", func() { + basePreset := func(reviewers []githubv1alpha1.RequiredPullRequestReviewer) githubv1alpha1.RulesetPreset { + return githubv1alpha1.RulesetPreset{ + Spec: githubv1alpha1.RulesetPresetSpec{ + Name: "pr-reviewers-ruleset", + Enforcement: githubv1alpha1.RulesetEnforcementActive, + Target: TargetTypeBranch, + Conditions: &githubv1alpha1.RulesetConditions{ + RefName: &githubv1alpha1.RefNameCondition{ + Include: []string{"main"}, + }, + }, + Rules: githubv1alpha1.RulesetRules{ + PullRequest: &githubv1alpha1.PullRequestRule{ + RequiredReviewers: reviewers, + }, + }, + }, + } + } + + It("should map nil required reviewers to empty slice", func() { + preset := basePreset(nil) + ruleset, err := RulesetPresetToGithubRuleset(preset) + Expect(err).NotTo(HaveOccurred()) + Expect(ruleset.Rules.PullRequest).NotTo(BeNil()) + Expect(ruleset.Rules.PullRequest.RequiredReviewers).To(BeEmpty()) + }) + + It("should map a single required reviewer with ID", func() { + teamID := int64(42) + preset := basePreset([]githubv1alpha1.RequiredPullRequestReviewer{ + { + MinimumApprovals: 1, + FilePatterns: []string{"*.go", "*.yaml"}, + Reviewer: githubv1alpha1.PullRequestReviewerEntity{ + ID: &teamID, + Type: "Team", + }, + }, + }) + ruleset, err := RulesetPresetToGithubRuleset(preset) + Expect(err).NotTo(HaveOccurred()) + Expect(ruleset.Rules.PullRequest.RequiredReviewers).To(HaveLen(1)) + + r := ruleset.Rules.PullRequest.RequiredReviewers[0] + Expect(r.MinimumApprovals).To(Equal(new(1))) + Expect(r.FilePatterns).To(ConsistOf("*.go", "*.yaml")) + Expect(r.Reviewer).NotTo(BeNil()) + Expect(*r.Reviewer.ID).To(Equal(int64(42))) + Expect(string(*r.Reviewer.Type)).To(Equal("Team")) + }) + + It("should map minimum approvals of zero", func() { + teamID := int64(10) + preset := basePreset([]githubv1alpha1.RequiredPullRequestReviewer{ + { + MinimumApprovals: 0, + Reviewer: githubv1alpha1.PullRequestReviewerEntity{ + ID: &teamID, + Type: "Team", + }, + }, + }) + ruleset, err := RulesetPresetToGithubRuleset(preset) + Expect(err).NotTo(HaveOccurred()) + r := ruleset.Rules.PullRequest.RequiredReviewers[0] + Expect(r.MinimumApprovals).To(Equal(new(0))) + }) + + It("should map nil file patterns to nil slice", func() { + teamID := int64(10) + preset := basePreset([]githubv1alpha1.RequiredPullRequestReviewer{ + { + FilePatterns: nil, + Reviewer: githubv1alpha1.PullRequestReviewerEntity{ + ID: &teamID, + Type: "Team", + }, + }, + }) + ruleset, err := RulesetPresetToGithubRuleset(preset) + Expect(err).NotTo(HaveOccurred()) + r := ruleset.Rules.PullRequest.RequiredReviewers[0] + Expect(r.FilePatterns).To(BeNil()) + }) + + It("should map multiple required reviewers", func() { + teamID1 := int64(10) + teamID2 := int64(20) + preset := basePreset([]githubv1alpha1.RequiredPullRequestReviewer{ + { + MinimumApprovals: 2, + FilePatterns: []string{"src/**"}, + Reviewer: githubv1alpha1.PullRequestReviewerEntity{ID: &teamID1, Type: "Team"}, + }, + { + MinimumApprovals: 1, + FilePatterns: []string{"docs/**"}, + Reviewer: githubv1alpha1.PullRequestReviewerEntity{ID: &teamID2, Type: "Team"}, + }, + }) + ruleset, err := RulesetPresetToGithubRuleset(preset) + Expect(err).NotTo(HaveOccurred()) + Expect(ruleset.Rules.PullRequest.RequiredReviewers).To(HaveLen(2)) + + ids := []int64{ + *ruleset.Rules.PullRequest.RequiredReviewers[0].Reviewer.ID, + *ruleset.Rules.PullRequest.RequiredReviewers[1].Reviewer.ID, + } + Expect(ids).To(ConsistOf(int64(10), int64(20))) + }) + + It("should use resolved ID (not slug) for the reviewer", func() { + resolvedID := int64(99) + preset := basePreset([]githubv1alpha1.RequiredPullRequestReviewer{ + { + Reviewer: githubv1alpha1.PullRequestReviewerEntity{ + ID: &resolvedID, + Type: "Team", + }, + }, + }) + ruleset, err := RulesetPresetToGithubRuleset(preset) + Expect(err).NotTo(HaveOccurred()) + Expect(*ruleset.Rules.PullRequest.RequiredReviewers[0].Reviewer.ID).To(Equal(int64(99))) + }) + }) + + Context("RulesetsDiffer with required reviewers", func() { + teamReviewerType := github.RulesetReviewerType("Team") + + basePresetWithReviewers := func(reviewers []githubv1alpha1.RequiredPullRequestReviewer) githubv1alpha1.RulesetPreset { + return githubv1alpha1.RulesetPreset{ + Spec: githubv1alpha1.RulesetPresetSpec{ + Name: "test", + Enforcement: githubv1alpha1.RulesetEnforcementActive, + Target: TargetTypeBranch, + Conditions: &githubv1alpha1.RulesetConditions{ + RefName: &githubv1alpha1.RefNameCondition{Include: []string{"main"}}, + }, + Rules: githubv1alpha1.RulesetRules{ + PullRequest: &githubv1alpha1.PullRequestRule{ + RequiredReviewers: reviewers, + }, + }, + }, + } + } + + baseGithubWithReviewers := func(reviewers []*github.RulesetRequiredReviewer) github.RepositoryRuleset { + return github.RepositoryRuleset{ + Name: "test", + Enforcement: github.RulesetEnforcementActive, + Target: github.Ptr(github.RulesetTargetBranch), + Conditions: &github.RepositoryRulesetConditions{ + RefName: &github.RepositoryRulesetRefConditionParameters{Include: []string{"main"}}, + }, + Rules: &github.RepositoryRulesetRules{ + PullRequest: &github.PullRequestRuleParameters{ + RequiredReviewers: reviewers, + }, + }, + } + } + + It("should return false when both have no required reviewers", func() { + preset := basePresetWithReviewers(nil) + ghRuleset := baseGithubWithReviewers(nil) + Expect(RulesetsDiffer(preset, ghRuleset)).To(BeFalse()) + }) + + It("should return false when required reviewers match", func() { + teamID := int64(42) + minApprovals := 1 + preset := basePresetWithReviewers([]githubv1alpha1.RequiredPullRequestReviewer{ + { + MinimumApprovals: 1, + FilePatterns: []string{"*.go"}, + Reviewer: githubv1alpha1.PullRequestReviewerEntity{ID: &teamID, Type: "Team"}, + }, + }) + ghRuleset := baseGithubWithReviewers([]*github.RulesetRequiredReviewer{ + { + MinimumApprovals: &minApprovals, + FilePatterns: []string{"*.go"}, + Reviewer: &github.RulesetReviewer{ID: &teamID, Type: &teamReviewerType}, + }, + }) + Expect(RulesetsDiffer(preset, ghRuleset)).To(BeFalse()) + }) + + It("should return true when reviewer count differs", func() { + teamID := int64(42) + minApprovals := 1 + preset := basePresetWithReviewers([]githubv1alpha1.RequiredPullRequestReviewer{ + {Reviewer: githubv1alpha1.PullRequestReviewerEntity{ID: &teamID, Type: "Team"}}, + }) + ghRuleset := baseGithubWithReviewers([]*github.RulesetRequiredReviewer{ + {MinimumApprovals: &minApprovals, Reviewer: &github.RulesetReviewer{ID: &teamID, Type: &teamReviewerType}}, + {MinimumApprovals: &minApprovals, Reviewer: &github.RulesetReviewer{ID: github.Ptr(int64(99)), Type: &teamReviewerType}}, + }) + Expect(RulesetsDiffer(preset, ghRuleset)).To(BeTrue()) + }) + + It("should return true when reviewer ID differs", func() { + teamID := int64(42) + differentID := int64(99) + minApprovals := 1 + preset := basePresetWithReviewers([]githubv1alpha1.RequiredPullRequestReviewer{ + {Reviewer: githubv1alpha1.PullRequestReviewerEntity{ID: &teamID, Type: "Team"}}, + }) + ghRuleset := baseGithubWithReviewers([]*github.RulesetRequiredReviewer{ + {MinimumApprovals: &minApprovals, Reviewer: &github.RulesetReviewer{ID: &differentID, Type: &teamReviewerType}}, + }) + Expect(RulesetsDiffer(preset, ghRuleset)).To(BeTrue()) + }) + + It("should return true when minimum approvals differ", func() { + teamID := int64(42) + minApprovals := 2 + preset := basePresetWithReviewers([]githubv1alpha1.RequiredPullRequestReviewer{ + {MinimumApprovals: 1, Reviewer: githubv1alpha1.PullRequestReviewerEntity{ID: &teamID, Type: "Team"}}, + }) + ghRuleset := baseGithubWithReviewers([]*github.RulesetRequiredReviewer{ + {MinimumApprovals: &minApprovals, Reviewer: &github.RulesetReviewer{ID: &teamID, Type: &teamReviewerType}}, + }) + Expect(RulesetsDiffer(preset, ghRuleset)).To(BeTrue()) + }) + + It("should return true when file patterns differ", func() { + teamID := int64(42) + minApprovals := 0 + preset := basePresetWithReviewers([]githubv1alpha1.RequiredPullRequestReviewer{ + {FilePatterns: []string{"*.go"}, Reviewer: githubv1alpha1.PullRequestReviewerEntity{ID: &teamID, Type: "Team"}}, + }) + ghRuleset := baseGithubWithReviewers([]*github.RulesetRequiredReviewer{ + {MinimumApprovals: &minApprovals, FilePatterns: []string{"*.yaml"}, Reviewer: &github.RulesetReviewer{ID: &teamID, Type: &teamReviewerType}}, + }) + Expect(RulesetsDiffer(preset, ghRuleset)).To(BeTrue()) + }) + + It("should return false when minimum approvals is nil on GitHub side and 0 in preset", func() { + teamID := int64(42) + preset := basePresetWithReviewers([]githubv1alpha1.RequiredPullRequestReviewer{ + {MinimumApprovals: 0, Reviewer: githubv1alpha1.PullRequestReviewerEntity{ID: &teamID, Type: "Team"}}, + }) + ghRuleset := baseGithubWithReviewers([]*github.RulesetRequiredReviewer{ + {MinimumApprovals: nil, Reviewer: &github.RulesetReviewer{ID: &teamID, Type: &teamReviewerType}}, + }) + Expect(RulesetsDiffer(preset, ghRuleset)).To(BeFalse()) + }) + + It("should return true when preset has reviewers but GitHub has none", func() { + teamID := int64(42) + preset := basePresetWithReviewers([]githubv1alpha1.RequiredPullRequestReviewer{ + {Reviewer: githubv1alpha1.PullRequestReviewerEntity{ID: &teamID, Type: "Team"}}, + }) + ghRuleset := baseGithubWithReviewers(nil) + Expect(RulesetsDiffer(preset, ghRuleset)).To(BeTrue()) + }) + + It("should return true when GitHub has reviewers but preset has none", func() { + teamID := int64(42) + minApprovals := 0 + preset := basePresetWithReviewers(nil) + ghRuleset := baseGithubWithReviewers([]*github.RulesetRequiredReviewer{ + {MinimumApprovals: &minApprovals, Reviewer: &github.RulesetReviewer{ID: &teamID, Type: &teamReviewerType}}, + }) + Expect(RulesetsDiffer(preset, ghRuleset)).To(BeTrue()) + }) + }) + Context("RulesetPresetToGithubRuleset with workflows", func() { It("should map workflows rule correctly", func() { preset := githubv1alpha1.RulesetPreset{ diff --git a/internal/reconciler/shared.go b/internal/reconciler/shared.go index d2356f8..596cbc1 100644 --- a/internal/reconciler/shared.go +++ b/internal/reconciler/shared.go @@ -34,6 +34,39 @@ func ResolveNamesToIDsInRuleset(ctx context.Context, githubClient ghclient.GitHu rs = resolveStatusCheckAppSlugs(orgName, installations, rs) + rs, err = resolveRequiredReviewerSlugs(ctx, githubClient, orgName, rs) + if err != nil { + return rs, err + } + + return rs, nil +} + +// resolveRequiredReviewerSlugs resolves slugs in pull request required reviewer entries to their numeric IDs. +func resolveRequiredReviewerSlugs(ctx context.Context, githubClient ghclient.GitHubClient, orgName string, rs v1alpha1.RulesetPreset) (v1alpha1.RulesetPreset, error) { + if rs.Spec.Rules.PullRequest == nil { + return rs, nil + } + updated := make([]v1alpha1.RequiredPullRequestReviewer, len(rs.Spec.Rules.PullRequest.RequiredReviewers)) + for i, reviewer := range rs.Spec.Rules.PullRequest.RequiredReviewers { + if reviewer.Reviewer.ID == nil && reviewer.Reviewer.Slug != nil { + reviewerType := github.RulesetReviewerType(reviewer.Reviewer.Type) + var resolver slugResolverFunc + switch reviewerType { + case github.RulesetReviewerTypeTeam: + resolver = teamSlugResolver(ctx, githubClient, orgName) + default: + return rs, fmt.Errorf("unsupported required reviewer type %q for slug resolution", reviewer.Reviewer.Type) + } + id, err := resolver(*reviewer.Reviewer.Slug) + if err != nil { + return rs, fmt.Errorf("failed to resolve required reviewer slug %q to ID: %w", *reviewer.Reviewer.Slug, err) + } + reviewer.Reviewer.ID = id + } + updated[i] = reviewer + } + rs.Spec.Rules.PullRequest.RequiredReviewers = updated return rs, nil } diff --git a/internal/reconciler/shared_test.go b/internal/reconciler/shared_test.go index e641125..a0a7872 100644 --- a/internal/reconciler/shared_test.go +++ b/internal/reconciler/shared_test.go @@ -953,4 +953,249 @@ var _ = Describe("ResolveNamesToIDsInRuleset", func() { Expect(mockClient.EnterpriseAppsCalls).To(HaveLen(1)) }) }) + + Context("when resolving required reviewer slugs", func() { + BeforeEach(func() { + mockClient.GetGitHubAppsInstallationsFunc = func(ctx context.Context, org string) ([]*github.Installation, error) { + return []*github.Installation{}, nil + } + }) + + Context("when there is no pull request rule", func() { + It("should succeed without calling GetTeamBySlug", func() { + Expect(err).NotTo(HaveOccurred()) + Expect(mockClient.TeamCalls).To(BeEmpty()) + }) + }) + + Context("when pull request rule has no required reviewers", func() { + BeforeEach(func() { + rulesetInput.Spec.Rules.PullRequest = &v1alpha1.PullRequestRule{} + }) + + It("should succeed without calling GetTeamBySlug", func() { + Expect(err).NotTo(HaveOccurred()) + Expect(mockClient.TeamCalls).To(BeEmpty()) + Expect(result.Spec.Rules.PullRequest.RequiredReviewers).To(BeEmpty()) + }) + }) + + Context("when reviewer already has an ID set", func() { + BeforeEach(func() { + rulesetInput.Spec.Rules.PullRequest = &v1alpha1.PullRequestRule{ + RequiredReviewers: []v1alpha1.RequiredPullRequestReviewer{ + { + Reviewer: v1alpha1.PullRequestReviewerEntity{ + ID: new(int64(42)), + Type: "Team", + }, + }, + }, + } + }) + + It("should preserve the existing ID without calling GetTeamBySlug", func() { + Expect(err).NotTo(HaveOccurred()) + Expect(mockClient.TeamCalls).To(BeEmpty()) + Expect(result.Spec.Rules.PullRequest.RequiredReviewers[0].Reviewer.ID).To(Equal(new(int64(42)))) + }) + }) + + Context("when reviewer has neither ID nor slug", func() { + BeforeEach(func() { + rulesetInput.Spec.Rules.PullRequest = &v1alpha1.PullRequestRule{ + RequiredReviewers: []v1alpha1.RequiredPullRequestReviewer{ + { + Reviewer: v1alpha1.PullRequestReviewerEntity{ + Type: "Team", + }, + }, + }, + } + }) + + It("should leave the ID as nil without error", func() { + Expect(err).NotTo(HaveOccurred()) + Expect(mockClient.TeamCalls).To(BeEmpty()) + Expect(result.Spec.Rules.PullRequest.RequiredReviewers[0].Reviewer.ID).To(BeNil()) + }) + }) + + Context("when reviewer has a Team slug", func() { + BeforeEach(func() { + rulesetInput.Spec.Rules.PullRequest = &v1alpha1.PullRequestRule{ + RequiredReviewers: []v1alpha1.RequiredPullRequestReviewer{ + { + MinimumApprovals: 1, + FilePatterns: []string{"*.go"}, + Reviewer: v1alpha1.PullRequestReviewerEntity{ + Slug: new("platform-team"), + Type: "Team", + }, + }, + }, + } + + mockClient.GetTeamBySlugFunc = func(ctx context.Context, org string, slug string) (*github.Team, error) { + Expect(org).To(Equal(orgName)) + Expect(slug).To(Equal("platform-team")) + return &github.Team{ + ID: new(int64(99)), + Slug: new("platform-team"), + }, nil + } + }) + + It("should resolve the slug to an ID", func() { + Expect(err).NotTo(HaveOccurred()) + Expect(result.Spec.Rules.PullRequest.RequiredReviewers).To(HaveLen(1)) + Expect(result.Spec.Rules.PullRequest.RequiredReviewers[0].Reviewer.ID).To(Equal(new(int64(99)))) + Expect(result.Spec.Rules.PullRequest.RequiredReviewers[0].Reviewer.Type).To(Equal("Team")) + Expect(result.Spec.Rules.PullRequest.RequiredReviewers[0].MinimumApprovals).To(Equal(1)) + Expect(result.Spec.Rules.PullRequest.RequiredReviewers[0].FilePatterns).To(Equal([]string{"*.go"})) + }) + + It("should call GetTeamBySlug once with the correct org and slug", func() { + Expect(err).NotTo(HaveOccurred()) + Expect(mockClient.TeamCalls).To(HaveLen(1)) + Expect(mockClient.TeamCalls[0].Method).To(Equal("GetTeamBySlug")) + Expect(mockClient.TeamCalls[0].Org).To(Equal(orgName)) + Expect(mockClient.TeamCalls[0].Slug).To(Equal("platform-team")) + }) + }) + + Context("when GetTeamBySlug returns an error for a reviewer slug", func() { + BeforeEach(func() { + rulesetInput.Spec.Rules.PullRequest = &v1alpha1.PullRequestRule{ + RequiredReviewers: []v1alpha1.RequiredPullRequestReviewer{ + { + Reviewer: v1alpha1.PullRequestReviewerEntity{ + Slug: new("nonexistent-team"), + Type: "Team", + }, + }, + }, + } + + mockClient.GetTeamBySlugFunc = func(ctx context.Context, org string, slug string) (*github.Team, error) { + return nil, errors.New("team not found") + } + }) + + It("should return an error containing the slug and the underlying error", func() { + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("nonexistent-team")) + Expect(err.Error()).To(ContainSubstring("team not found")) + }) + }) + + Context("when reviewer has an unsupported type with a slug", func() { + BeforeEach(func() { + rulesetInput.Spec.Rules.PullRequest = &v1alpha1.PullRequestRule{ + RequiredReviewers: []v1alpha1.RequiredPullRequestReviewer{ + { + Reviewer: v1alpha1.PullRequestReviewerEntity{ + Slug: new("some-slug"), + Type: "UnknownType", + }, + }, + }, + } + }) + + It("should return an unsupported type error", func() { + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("unsupported required reviewer type")) + Expect(err.Error()).To(ContainSubstring("UnknownType")) + }) + }) + + Context("when multiple reviewers have mixed ID and slug", func() { + BeforeEach(func() { + rulesetInput.Spec.Rules.PullRequest = &v1alpha1.PullRequestRule{ + RequiredReviewers: []v1alpha1.RequiredPullRequestReviewer{ + { + Reviewer: v1alpha1.PullRequestReviewerEntity{ + ID: new(int64(10)), + Type: "Team", + }, + }, + { + Reviewer: v1alpha1.PullRequestReviewerEntity{ + Slug: new("backend-team"), + Type: "Team", + }, + }, + { + Reviewer: v1alpha1.PullRequestReviewerEntity{ + Slug: new("security-team"), + Type: "Team", + }, + }, + }, + } + + mockClient.GetTeamBySlugFunc = func(ctx context.Context, org string, slug string) (*github.Team, error) { + switch slug { + case "backend-team": + return &github.Team{ID: new(int64(20)), Slug: new("backend-team")}, nil + case "security-team": + return &github.Team{ID: new(int64(30)), Slug: new("security-team")}, nil + } + return nil, errors.New("team not found") + } + }) + + It("should resolve all slugs and preserve existing IDs", func() { + Expect(err).NotTo(HaveOccurred()) + Expect(result.Spec.Rules.PullRequest.RequiredReviewers).To(HaveLen(3)) + Expect(result.Spec.Rules.PullRequest.RequiredReviewers[0].Reviewer.ID).To(Equal(new(int64(10)))) + Expect(result.Spec.Rules.PullRequest.RequiredReviewers[1].Reviewer.ID).To(Equal(new(int64(20)))) + Expect(result.Spec.Rules.PullRequest.RequiredReviewers[2].Reviewer.ID).To(Equal(new(int64(30)))) + }) + + It("should call GetTeamBySlug only for the slug-based reviewers", func() { + Expect(err).NotTo(HaveOccurred()) + Expect(mockClient.TeamCalls).To(HaveLen(2)) + }) + }) + + Context("when bypass actors and required reviewer slugs both need resolution", func() { + BeforeEach(func() { + rulesetInput.Spec.BypassActors = []v1alpha1.RulesetBypassActor{ + { + ActorSlug: new("platform-team"), + ActorType: "Team", + BypassMode: "always", + }, + } + rulesetInput.Spec.Rules.PullRequest = &v1alpha1.PullRequestRule{ + RequiredReviewers: []v1alpha1.RequiredPullRequestReviewer{ + { + Reviewer: v1alpha1.PullRequestReviewerEntity{ + Slug: new("security-team"), + Type: "Team", + }, + }, + }, + } + + mockClient.GetTeamBySlugFunc = func(ctx context.Context, org string, slug string) (*github.Team, error) { + switch slug { + case "platform-team": + return &github.Team{ID: new(int64(11)), Slug: new("platform-team")}, nil + case "security-team": + return &github.Team{ID: new(int64(22)), Slug: new("security-team")}, nil + } + return nil, errors.New("team not found") + } + }) + + It("should resolve both bypass actor and reviewer slugs independently", func() { + Expect(err).NotTo(HaveOccurred()) + Expect(result.Spec.BypassActors[0].ActorID).To(Equal(new(int64(11)))) + Expect(result.Spec.Rules.PullRequest.RequiredReviewers[0].Reviewer.ID).To(Equal(new(int64(22)))) + }) + }) + }) }) From cff15bd777da676f4490cd9bb1dfcb1ac934f12c Mon Sep 17 00:00:00 2001 From: tstollin Date: Wed, 15 Jul 2026 11:41:57 +0200 Subject: [PATCH 2/2] fix: gate required_reviewers pull_request rule reconciliation by feature flag also centralizes loading of env-based configuration --- README.md | 12 +++ api/v1alpha1/organization_methods.go | 85 ++++++++++--------- api/v1alpha1/repository_methods.go | 32 +++---- api/v1alpha1/team_methods.go | 30 +++---- cmd/main.go | 68 ++++++--------- docs/architecture.md | 19 ++++- docs/configuration/repository.md | 15 ++++ go.mod | 1 + go.sum | 2 + internal/config/config.go | 50 +++++++++++ .../organization_controller_test.go | 1 - .../controller/repository_controller_test.go | 3 +- internal/controller/team_controller_test.go | 7 +- internal/ghclient/factory.go | 76 ++++++++++------- internal/reconciler/orgrec/rec_rulesets.go | 6 ++ internal/reconciler/orgrec/reconciler.go | 2 + .../reconciler/reconcilerfactory/factory.go | 40 ++++++--- .../reconcilerfactory/factory_test.go | 8 +- internal/reconciler/reporec/rec_rulesets.go | 5 ++ internal/reconciler/reporec/reconciler.go | 2 + internal/reconciler/spreading/spreading.go | 79 ++++++++++------- .../reconciler/spreading/spreading_test.go | 81 +++++------------- internal/reconciler/teamrec/rec_members.go | 3 +- .../reconciler/teamrec/rec_members_test.go | 4 +- internal/reconciler/teamrec/reconciler.go | 4 + internal/reconciler/types.go | 3 +- internal/utils/utils.go | 10 --- .../webhook/v1alpha1/repository_webhook.go | 17 ++-- .../v1alpha1/repository_webhook_test.go | 1 - .../webhook/v1alpha1/webhook_suite_test.go | 2 +- test/mock/ghclientmock/mock_factory.go | 5 +- 31 files changed, 384 insertions(+), 289 deletions(-) create mode 100644 internal/config/config.go diff --git a/README.md b/README.md index 314b450..c192bd6 100644 --- a/README.md +++ b/README.md @@ -212,6 +212,18 @@ Common patterns: - **Organizations**: Only deleted when no `Repository` references remain (enforced via finalizer) - **Repositories**: Archived instead of hard-deleted +### Feature Flags + +Boolean environment variables that enable or disable operator functionality, loaded once at startup. Invalid values cause the operator to exit with a clear error. + +| Variable | Default | Description | +|----------|---------|-------------| +| `ENABLE_STARTUP_SPREADING` | `true` | Enable startup spreading to prevent API rate-limit exhaustion after pod restarts | +| `ENABLE_WEBHOOKS` | `true` | Enable the admission webhook server. Set `false` for local development without cert-manager | +| `ENABLE_REQUIRED_REVIEWERS_RULES` | `false` | Enable `requiredReviewers` in pull-request ruleset rules (GitHub API feature is in **beta**) | + +See [Architecture → Feature Flags](https://interhyp.github.io/git-hubby/architecture/#feature-flags) for details. + ## Documentation For detailed configuration and usage information, see the [full documentation](https://interhyp.github.io/git-hubby/): diff --git a/api/v1alpha1/organization_methods.go b/api/v1alpha1/organization_methods.go index 9c5fb45..c4f0139 100644 --- a/api/v1alpha1/organization_methods.go +++ b/api/v1alpha1/organization_methods.go @@ -24,67 +24,67 @@ const ( VisibilityInternal = "internal" ) -func (in *Organization) GetTypeRepresentation() string { +func (o *Organization) GetTypeRepresentation() string { return "Organization" } -func (in *Organization) GetConditions() *[]metav1.Condition { - if in == nil { +func (o *Organization) GetConditions() *[]metav1.Condition { + if o == nil { return nil } - return &in.Status.Conditions + return &o.Status.Conditions } -func (in *Organization) IsHealthy() bool { - if in == nil { +func (o *Organization) IsHealthy() bool { + if o == nil { return false } - readyCondition := meta.FindStatusCondition(in.Status.Conditions, string(conditions.TypeReady)) + readyCondition := meta.FindStatusCondition(o.Status.Conditions, string(conditions.TypeReady)) return readyCondition != nil && readyCondition.Status == metav1.ConditionTrue } -func (in *Organization) GetObservedGeneration() int64 { - if in == nil { +func (o *Organization) GetObservedGeneration() int64 { + if o == nil { return 0 } - readyCondition := meta.FindStatusCondition(in.Status.Conditions, string(conditions.TypeReady)) + readyCondition := meta.FindStatusCondition(o.Status.Conditions, string(conditions.TypeReady)) if readyCondition == nil { return 0 } return readyCondition.ObservedGeneration } -func (in *Organization) GetObservedSubResourceGenerations() map[string]int64 { - if in == nil { +func (o *Organization) GetObservedSubResourceGenerations() map[string]int64 { + if o == nil { return nil } - return in.Status.ObservedSubResourceGenerations + return o.Status.ObservedSubResourceGenerations } -func (in *Organization) GetPlan() string { - if in == nil { +func (o *Organization) GetPlan() string { + if o == nil { return "" } - if in.Spec.Plan == "" { + if o.Spec.Plan == "" { return PlanEnterprise } - return in.Spec.Plan + return o.Spec.Plan } // HasEnterpriseFeatures returns true if the organization has enterprise-level features. // Returns false for free plan, true for enterprise and other plans. -func (in *Organization) HasEnterpriseFeatures() bool { - if in == nil { +func (o *Organization) HasEnterpriseFeatures() bool { + if o == nil { return false } - return in.GetPlan() != PlanFree + return o.GetPlan() != PlanFree } -func (in *Organization) SetObservedSubResourceGeneration(new map[string]int64) { - if in == nil { +func (o *Organization) SetObservedSubResourceGeneration(new map[string]int64) { + if o == nil { return } - in.Status.ObservedSubResourceGenerations = new + o.Status.ObservedSubResourceGenerations = new } func (o *OrgCustomPropertyDefaultValue) GetValue() *string { @@ -139,23 +139,32 @@ func (o *Organization) IsUsingLegacyNameField() bool { return o.Spec.Login == "" && o.Spec.Name != "" } -// ResolveGitHubAppConfig resolves the effective GitHubAppConfig for this Organization. -// If GitHubAppConfig is set, it is returned directly (it takes precedence). -// Otherwise, if the deprecated GitHubAppInstallationId field is set, a GitHubAppConfig is -// synthesised using the provided legacySecretName as the credentials secret. +// GetGitHubAppInstallationID returns the effective GitHub App installation ID for this Organization. +// spec.githubAppConfig takes precedence over the deprecated spec.githubAppInstallationId field. // Returns an error if neither field is set. -func (in *Organization) ResolveGitHubAppConfig(legacySecretName string) (*GitHubAppConfig, error) { - if in == nil { - return nil, fmt.Errorf("organization is nil") +func (o *Organization) GetGitHubAppInstallationID() (int64, error) { + if o == nil { + return 0, fmt.Errorf("organization is nil") + } + if o.Spec.GitHubAppConfig != nil { + return o.Spec.GitHubAppConfig.InstallationId, nil } - if in.Spec.GitHubAppConfig != nil { - return in.Spec.GitHubAppConfig, nil + if o.Spec.GitHubAppInstallationId != nil { + return *o.Spec.GitHubAppInstallationId, nil + } + return 0, fmt.Errorf("organization %s/%s has neither githubAppConfig nor githubAppInstallationId set", o.Namespace, o.Name) +} + +// GetGitHubAppCredentialsSecretName returns the credentials secret name for this Organization, +// or an empty string when using the deprecated spec.githubAppInstallationId field. +// An empty return value means the GitHub client manager should fall back to its configured +// legacy secret name. +func (o *Organization) GetGitHubAppCredentialsSecretName() string { + if o == nil { + return "" } - if in.Spec.GitHubAppInstallationId != nil { - return &GitHubAppConfig{ - InstallationId: *in.Spec.GitHubAppInstallationId, - CredentialsSecretName: legacySecretName, - }, nil + if o.Spec.GitHubAppConfig != nil { + return o.Spec.GitHubAppConfig.CredentialsSecretName } - return nil, fmt.Errorf("organization %s/%s has neither githubAppConfig nor githubAppInstallationId set", in.Namespace, in.Name) + return "" } diff --git a/api/v1alpha1/repository_methods.go b/api/v1alpha1/repository_methods.go index 5bc6e08..665ab3f 100644 --- a/api/v1alpha1/repository_methods.go +++ b/api/v1alpha1/repository_methods.go @@ -6,48 +6,48 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) -func (in *Repository) GetConditions() *[]metav1.Condition { - if in == nil { +func (r *Repository) GetConditions() *[]metav1.Condition { + if r == nil { return nil } - return &in.Status.Conditions + return &r.Status.Conditions } -func (in *Repository) GetTypeRepresentation() string { +func (r *Repository) GetTypeRepresentation() string { return "Repository" } -func (in *Repository) IsHealthy() bool { - if in == nil { +func (r *Repository) IsHealthy() bool { + if r == nil { return false } - readyCondition := meta.FindStatusCondition(in.Status.Conditions, "Ready") + readyCondition := meta.FindStatusCondition(r.Status.Conditions, "Ready") return readyCondition != nil && readyCondition.Status == metav1.ConditionTrue } -func (in *Repository) GetObservedGeneration() int64 { - if in == nil { +func (r *Repository) GetObservedGeneration() int64 { + if r == nil { return 0 } - readyCondition := meta.FindStatusCondition(in.Status.Conditions, string(conditions.TypeReady)) + readyCondition := meta.FindStatusCondition(r.Status.Conditions, string(conditions.TypeReady)) if readyCondition == nil { return 0 } return readyCondition.ObservedGeneration } -func (in *Repository) GetObservedSubResourceGenerations() map[string]int64 { - if in == nil { +func (r *Repository) GetObservedSubResourceGenerations() map[string]int64 { + if r == nil { return nil } - return in.Status.ObservedSubResourceGenerations + return r.Status.ObservedSubResourceGenerations } -func (in *Repository) SetObservedSubResourceGeneration(new map[string]int64) { - if in == nil { +func (r *Repository) SetObservedSubResourceGeneration(new map[string]int64) { + if r == nil { return } - in.Status.ObservedSubResourceGenerations = new + r.Status.ObservedSubResourceGenerations = new } func (c *CustomPropertyValue) GetValue() *string { diff --git a/api/v1alpha1/team_methods.go b/api/v1alpha1/team_methods.go index 0ea5784..1521959 100644 --- a/api/v1alpha1/team_methods.go +++ b/api/v1alpha1/team_methods.go @@ -6,45 +6,45 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) -func (in *Team) GetConditions() *[]metav1.Condition { - if in == nil { +func (t *Team) GetConditions() *[]metav1.Condition { + if t == nil { return nil } - return &in.Status.Conditions + return &t.Status.Conditions } -func (in *Team) GetTypeRepresentation() string { +func (t *Team) GetTypeRepresentation() string { return "Team" } -func (in *Team) IsIDPTeam() bool { - if in == nil { +func (t *Team) IsIDPTeam() bool { + if t == nil { return false } - return in.Spec.IDPGroup != nil + return t.Spec.IDPGroup != nil } -func (in *Team) IsHealthy() bool { - if in == nil { +func (t *Team) IsHealthy() bool { + if t == nil { return false } - readyCondition := meta.FindStatusCondition(in.Status.Conditions, "Ready") + readyCondition := meta.FindStatusCondition(t.Status.Conditions, "Ready") return readyCondition != nil && readyCondition.Status == metav1.ConditionTrue } -func (in *Team) GetObservedGeneration() int64 { - if in == nil { +func (t *Team) GetObservedGeneration() int64 { + if t == nil { return 0 } - readyCondition := meta.FindStatusCondition(in.Status.Conditions, string(conditions.TypeReady)) + readyCondition := meta.FindStatusCondition(t.Status.Conditions, string(conditions.TypeReady)) if readyCondition == nil { return 0 } return readyCondition.ObservedGeneration } -func (in *Team) GetObservedSubResourceGenerations() map[string]int64 { +func (t *Team) GetObservedSubResourceGenerations() map[string]int64 { return nil } -func (in *Team) SetObservedSubResourceGeneration(_ map[string]int64) {} +func (t *Team) SetObservedSubResourceGeneration(_ map[string]int64) {} diff --git a/cmd/main.go b/cmd/main.go index d71ce4b..980b50d 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -21,6 +21,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/healthz" + "github.com/Interhyp/git-hubby/internal/config" "github.com/Interhyp/git-hubby/internal/reconciler/reconcilerfactory" "github.com/Interhyp/git-hubby/internal/ghclient" @@ -57,21 +58,6 @@ func init() { // +kubebuilder:scaffold:scheme } -// getWatchNamespace returns the namespace(s) the manager should watch for changes. -// It reads the value from the WATCH_NAMESPACE environment variable. -func getWatchNamespace() (string, error) { - watchNamespaceEnvVar := "WATCH_NAMESPACE" - ns, found := os.LookupEnv(watchNamespaceEnvVar) - if !found { - return "", fmt.Errorf("%s must be set", watchNamespaceEnvVar) - } - ns = strings.TrimSpace(ns) - if ns == "" { - return "", fmt.Errorf("%s must not be empty", watchNamespaceEnvVar) - } - return ns, nil -} - // setupCacheNamespaces configures the cache to watch specific namespace(s). // It returns an error if no valid namespaces remain after parsing. func setupCacheNamespaces(namespaces string) (cache.Options, error) { @@ -209,12 +195,21 @@ func main() { fmt.Fprintf(os.Stderr, "Error loading .env file: %v\n", err) os.Exit(1) } + + // Parse all environment configuration immediately after godotenv so that every + // subsequent initialisation step (including logger setup) reads from cfg. + cfg, err := config.Parse() + if err != nil { + fmt.Fprintf(os.Stderr, "Unable to parse configuration from environment: %v\n", err) + os.Exit(1) + } + // Support LOG_LEVEL env var (overrides --zap-log-level flag) - if level, ok := parseLogLevel(os.Getenv("LOG_LEVEL")); ok { + if level, ok := parseLogLevel(cfg.LogLevel); ok { opts.Level = level } - ctrl.SetLogger(zap.New(buildLoggerOpts(&opts, parseLogFormat(os.Getenv("LOG_FORMAT")))...)) + ctrl.SetLogger(zap.New(buildLoggerOpts(&opts, parseLogFormat(cfg.LogFormat))...)) // if the enable-http2 flag is false (the default), http/2 should be disabled // due to its vulnerabilities. More specifically, disabling http/2 will @@ -283,21 +278,7 @@ func main() { metricsServerOptions.KeyName = metricsCertKey } - // Get the namespace(s) for namespace-scoped mode from WATCH_NAMESPACE environment variable. - watchNamespace, err := getWatchNamespace() - if err != nil { - setupLog.Error(err, "Unable to get WATCH_NAMESPACE") - os.Exit(1) - } - - // APP_CREDENTIALS_SECRET_NAMESPACE specifies the namespace containing the GitHub App credentials secret. - // In the Helm chart this defaults to the release namespace (where the controller is deployed). - appCredentialsSecretNamespace := os.Getenv("APP_CREDENTIALS_SECRET_NAMESPACE") - if strings.TrimSpace(appCredentialsSecretNamespace) == "" { - setupLog.Error(fmt.Errorf("APP_CREDENTIALS_SECRET_NAMESPACE must be set"), "Unable to determine secret namespace") - os.Exit(1) - } - setupLog.Info("App credentials secret namespace configured", "namespace", appCredentialsSecretNamespace) + setupLog.Info("App credentials secret namespace configured", "namespace", cfg.AppCredentialsSecretNamespace) mgrOptions := ctrl.Options{ Scheme: scheme, @@ -321,13 +302,13 @@ func main() { } // Configure cache to watch namespace(s) specified in WATCH_NAMESPACE - cacheOpts, err := setupCacheNamespaces(watchNamespace) + cacheOpts, err := setupCacheNamespaces(cfg.WatchNamespace) if err != nil { setupLog.Error(err, "Invalid WATCH_NAMESPACE configuration") os.Exit(1) } mgrOptions.Cache = cacheOpts - setupLog.Info("Watching namespace(s)", "namespaces", watchNamespace) + setupLog.Info("Watching namespace(s)", "namespaces", cfg.WatchNamespace) mgr, err := ctrl.NewManager(ctrl.GetConfigOrDie(), mgrOptions) if err != nil { @@ -350,7 +331,7 @@ func main() { var secret v1.Secret secretKey := client.ObjectKey{ Name: secretName, - Namespace: appCredentialsSecretNamespace, + Namespace: cfg.AppCredentialsSecretNamespace, } if fetchErr := directClient.Get(ctx, secretKey, &secret); fetchErr != nil { log.Error(fetchErr, "Failed to fetch secret", "secretName", secretName) @@ -367,7 +348,11 @@ func main() { setupLog.Error(err, "failed to create GitHub client factory") os.Exit(1) } - spreadingManager := spreading.NewDefaultManager() + spreadingManager := spreading.NewDefaultManager( + spreading.WithEnabled(cfg.Features.EnableStartupSpreading), + spreading.WithSpreadPeriod(cfg.SpreadPeriodMinutes), + spreading.WithSpreadInterval(cfg.SpreadIntervalMinutes), + ) setupLog.Info("Startup spreading configured", "spreadPeriod", spreadingManager.Config.SpreadPeriod, "spreadInterval", spreadingManager.Config.SpreadInterval, @@ -378,11 +363,9 @@ func main() { ClientManager: clientManager, SpreadingManager: spreadingManager, K8sClient: mgr.GetClient(), - LegacySecretName: appCredentialsSecretName, + Config: cfg, } - webhooksEnabled := os.Getenv("ENABLE_WEBHOOKS") != "false" - globalLimiter := ratelimit.NewGitHubRateLimiter(ratelimit.GitHubRateLimiterConfig{ RequestsPerHour: 15000, // GitHub's rate limit BurstSize: 500, // Allow some burst @@ -394,7 +377,6 @@ func main() { ReconcilerFactory: reconcilerFactory, GithubRateLimiter: globalLimiter, SuccessRequeueInterval: spreadingManager.GetSpreadInterval(), - LegacySecretName: appCredentialsSecretName, }).SetupWithManager(mgr); err != nil { setupLog.Error(err, "unable to create controller", "controller", "Organization") os.Exit(1) @@ -417,14 +399,14 @@ func main() { setupLog.Error(err, "unable to create controller", "controller", "Team") os.Exit(1) } - if webhooksEnabled { + if cfg.Features.EnableWebhooks { setupLog.V(1).Info("Webhooks enabled") if err := webhookv1alpha1.SetupOrganizationWebhookWithManager(mgr); err != nil { setupLog.Error(err, "unable to create webhook", "webhook", "Organization") os.Exit(1) } if err := webhookv1alpha1.SetupRepositoryWebhookWithManager( - mgr, clientManager, appCredentialsSecretName, + mgr, clientManager, ); err != nil { setupLog.Error(err, "unable to create webhook", "webhook", "Repository") os.Exit(1) @@ -440,7 +422,7 @@ func main() { setupLog.Error(err, "unable to set up ready check") os.Exit(1) } - if webhooksEnabled { + if cfg.Features.EnableWebhooks { if err := mgr.AddReadyzCheck("webhook", mgr.GetWebhookServer().StartedChecker()); err != nil { setupLog.Error(err, "unable to set up webhook ready check") os.Exit(1) diff --git a/docs/architecture.md b/docs/architecture.md index 013b50c..821da3a 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -31,13 +31,14 @@ To prevent API rate limit exhaustion during pod restarts (e.g., rolling deployme ### Configuration -Control via environment variables: +Control spreading behaviour via environment variables: | Variable | Default | Description | |----------|---------|-------------| -| `ENABLE_STARTUP_SPREADING` | `true` | Enable/disable spreading | -| `STARTUP_SPREAD_PERIOD_MINUTES` | `5` | Window after startup for spreading | -| `SPREAD_INTERVAL_MINUTES` | `180` | Time window for distribution | +| `STARTUP_SPREAD_PERIOD_MINUTES` | `5` | Window in minutes after startup during which warm-start reconciliations may be delayed | +| `SPREAD_INTERVAL_MINUTES` | `180` | Time window in minutes across which delayed reconciliations are distributed | + +> **Note**: Whether spreading is enabled at all is controlled by the `ENABLE_STARTUP_SPREADING` feature flag (see [Feature Flags](#feature-flags) below). ## Parallel Reconciliation @@ -81,3 +82,13 @@ The `GitHubCachingClientFactory` maintains a per-process cache of authenticated - Clients are cached per GitHub App installation - Memory overhead is minimal - Automatic token refresh on expiration + +## Feature Flags + +Feature flags are boolean environment variables that enable or disable operator functionality. They are all loaded once at startup via the `internal/features` package (using [`caarlos0/env`](https://github.com/caarlos0/env)) and passed through the reconciler factory. Invalid values (e.g. a non-boolean string) cause the operator to exit at startup with a clear error message. + +| Variable | Default | Description | +|----------|---------|-------------| +| `ENABLE_STARTUP_SPREADING` | `true` | Enable the startup spreading mechanism that distributes warm-start reconciliations over time to prevent API rate-limit exhaustion after pod restarts | +| `ENABLE_WEBHOOKS` | `true` | Enable registration of the admission webhook server. Set to `false` for local development without cert-manager (`make run` does this automatically) | +| `ENABLE_REQUIRED_REVIEWERS_RULES` | `false` | Enable reconciliation of `requiredReviewers` in pull-request ruleset rules. The underlying GitHub API is currently in **beta**; opt in explicitly when ready | diff --git a/docs/configuration/repository.md b/docs/configuration/repository.md index 7c4c5e0..4344761 100644 --- a/docs/configuration/repository.md +++ b/docs/configuration/repository.md @@ -203,6 +203,14 @@ rulesets: rules: pullRequest: requiredApprovingReviewCount: 2 + # requiredReviewers requires ENABLE_REQUIRED_REVIEWERS_RULES=true (GitHub beta feature) + requiredReviewers: + - minimumApprovals: 1 + filePatterns: + - "src/**" + reviewer: + slug: security-team # resolved to ID at reconciliation time + type: Team requiredStatusChecks: requiredStatusChecks: - context: "ci/build" @@ -211,6 +219,13 @@ rulesets: nonFastForward: true # Prevent force-push ``` +> **Note**: The `requiredReviewers` field in pull-request rules uses a GitHub API feature that is +> currently in **beta**. Reconciliation of this field is disabled by default and must be explicitly +> opted in by setting the environment variable `ENABLE_REQUIRED_REVIEWERS_RULES=true` on the +> controller manager. When the flag is not set, any `requiredReviewers` entries defined in the spec +> are ignored and will not be applied to or compared against GitHub. +> See [Feature Flags](../architecture.md#feature-flags) for all available flags. + ### Deploy Keys SSH keys for CI/CD systems to access the repository: diff --git a/go.mod b/go.mod index da88f5a..d475a5a 100644 --- a/go.mod +++ b/go.mod @@ -38,6 +38,7 @@ require ( github.com/antlr4-go/antlr/v4 v4.13.0 // indirect github.com/beorn7/perks v1.0.1 // indirect github.com/blang/semver/v4 v4.0.0 // indirect + github.com/caarlos0/env/v11 v11.4.1 // indirect github.com/cenkalti/backoff/v5 v5.0.3 // indirect github.com/cespare/xxhash/v2 v2.3.0 // indirect github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc // indirect diff --git a/go.sum b/go.sum index f28be7e..b08ec03 100644 --- a/go.sum +++ b/go.sum @@ -16,6 +16,8 @@ github.com/blang/semver/v4 v4.0.0 h1:1PFHFE6yCCTv8C1TeyNNarDzntLi7wMI5i/pzqYIsAM github.com/blang/semver/v4 v4.0.0/go.mod h1:IbckMUScFkM3pff0VJDNKRiT6TG/YpiHIM2yvyW5YoQ= github.com/bradleyfalzon/ghinstallation/v2 v2.19.0 h1:KQfD+43pRw9NUJhGycGrFr9vF1MubZacksKol1gomFI= github.com/bradleyfalzon/ghinstallation/v2 v2.19.0/go.mod h1:fe5ECIhCdEnxwLiBlNTxx9CP455wt42BELnlDVMvaAA= +github.com/caarlos0/env/v11 v11.4.1 h1:fYwH0sWEsBSMPG7t4e/PEfTFzrWrpjyygXyUnWiSwEw= +github.com/caarlos0/env/v11 v11.4.1/go.mod h1:qupehSf/Y0TUTsxKywqRt/vJjN5nz6vauiYEUUr8P4U= github.com/cenkalti/backoff/v5 v5.0.3 h1:ZN+IMa753KfX5hd8vVaMixjnqRZ3y8CuJKRKj1xcsSM= github.com/cenkalti/backoff/v5 v5.0.3/go.mod h1:rkhZdG3JZukswDf7f0cwqPNk4K0sa+F97BxZthm/crw= github.com/cespare/xxhash/v2 v2.3.0 h1:UL815xU9SqsFlibzuggzjXhog7bL6oX9BbNZnL2UFvs= diff --git a/internal/config/config.go b/internal/config/config.go new file mode 100644 index 0000000..9ba99cf --- /dev/null +++ b/internal/config/config.go @@ -0,0 +1,50 @@ +package config + +import ( + "github.com/caarlos0/env/v11" +) + +// Features holds all operator feature flags. Fields are populated from environment +// variables at startup via config.Parse and are immutable after construction. +type Features struct { + // EnableRequiredReviewersRules enables reconciliation of requiredReviewers in + // pull-request ruleset rules. Disabled by default; the GitHub API is in beta. + EnableRequiredReviewersRules bool `env:"ENABLE_REQUIRED_REVIEWERS_RULES" envDefault:"false"` + + // EnableWebhooks enables registration of the admission webhook server. + // Disable locally with ENABLE_WEBHOOKS=false to run without cert-manager. + EnableWebhooks bool `env:"ENABLE_WEBHOOKS" envDefault:"true"` + + // EnableStartupSpreading enables the startup spreading mechanism that + // distributes warm-start reconciliations over time to avoid API rate-limit exhaustion. + EnableStartupSpreading bool `env:"ENABLE_STARTUP_SPREADING" envDefault:"true"` +} + +// Config holds all operator configuration loaded from environment variables at startup. +type Config struct { + Features Features + + // Kubernetes scope + WatchNamespace string `env:"WATCH_NAMESPACE,notEmpty"` + AppCredentialsSecretNamespace string `env:"APP_CREDENTIALS_SECRET_NAMESPACE,notEmpty"` + + // Repository reconciliation + RepositoryFinalizerMode string `env:"REPOSITORY_FINALIZER_MODE"` + + // Team reconciliation + GitHubMemberSuffix string `env:"GITHUB_MEMBER_SUFFIX"` + + // Startup spreading — numeric tuning; defaults match spreading.DefaultSpreadPeriodMinutes + // and spreading.DefaultSpreadIntervalMinutes. + SpreadPeriodMinutes int `env:"STARTUP_SPREAD_PERIOD_MINUTES" envDefault:"5"` + SpreadIntervalMinutes int `env:"SPREAD_INTERVAL_MINUTES" envDefault:"180"` + + // Logging — consumed early in main.go before the structured logger is fully initialised. + LogLevel string `env:"LOG_LEVEL"` + LogFormat string `env:"LOG_FORMAT"` +} + +func Parse() (Config, error) { + c := Config{} + return c, env.Parse(&c) +} diff --git a/internal/controller/organization_controller_test.go b/internal/controller/organization_controller_test.go index 1f30947..fc7a7e6 100644 --- a/internal/controller/organization_controller_test.go +++ b/internal/controller/organization_controller_test.go @@ -37,7 +37,6 @@ var _ = Describe("Organization Controller - Integration Tests", func() { ClientManager: ghclientmock.NewGitHubMockClientFactory(mockClient), K8sClient: testEnv.Client, SpreadingManager: &mock.NoOpSpreadManager{}, - LegacySecretName: secretName, } // Create test namespace and secret testEnv.CreateTestNamespace(namespaceName) diff --git a/internal/controller/repository_controller_test.go b/internal/controller/repository_controller_test.go index 6a8d3df..826897f 100644 --- a/internal/controller/repository_controller_test.go +++ b/internal/controller/repository_controller_test.go @@ -42,7 +42,6 @@ var _ = Describe("Repository Controller - Integration Tests", func() { ClientManager: ghclientmock.NewGitHubMockClientFactory(mockClient), K8sClient: testEnv.Client, SpreadingManager: &mock.NoOpSpreadManager{}, - LegacySecretName: secretName, } testEnv.CreateTestNamespace(namespaceName) testEnv.CreateSecret(namespaceName, secretName) @@ -118,6 +117,8 @@ var _ = Describe("Repository Controller - Integration Tests", func() { ) BeforeEach(func() { + // Enable archive mode for deletion tests via the factory field. + factory.Config.RepositoryFinalizerMode = "archive" namespacedName = types.NamespacedName{ Name: repoName, Namespace: namespaceName, diff --git a/internal/controller/team_controller_test.go b/internal/controller/team_controller_test.go index 6131b14..2afdf9f 100644 --- a/internal/controller/team_controller_test.go +++ b/internal/controller/team_controller_test.go @@ -4,9 +4,9 @@ import ( "context" "fmt" "net/http" - "os" githubv1alpha1 "github.com/Interhyp/git-hubby/api/v1alpha1" + "github.com/Interhyp/git-hubby/internal/config" "github.com/Interhyp/git-hubby/internal/reconciler/reconcilerfactory" "github.com/Interhyp/git-hubby/test/mock" "github.com/Interhyp/git-hubby/test/mock/ghclientmock" @@ -37,11 +37,12 @@ var _ = Describe("TeamController", func() { ClientManager: ghclientmock.NewGitHubMockClientFactory(mockClient), K8sClient: testEnv.Client, SpreadingManager: &mock.NoOpSpreadManager{}, - LegacySecretName: "test-credentials", + Config: config.Config{ + GitHubMemberSuffix: "_memberSuffix", + }, } testEnv.CreateTestNamespace(namespaceName) _ = testEnv.SetupOrganizationTest(nil, namespaceName, orgName) - _ = os.Setenv("GITHUB_MEMBER_SUFFIX", "_memberSuffix") }) AfterEach(func() { diff --git a/internal/ghclient/factory.go b/internal/ghclient/factory.go index fb8eada..b777965 100644 --- a/internal/ghclient/factory.go +++ b/internal/ghclient/factory.go @@ -12,7 +12,6 @@ import ( "sync" "time" - "github.com/Interhyp/git-hubby/api/v1alpha1" "github.com/PuerkitoBio/rehttp" "github.com/gofri/go-github-pagination/githubpagination" "github.com/gofri/go-github-ratelimit/v2/github_ratelimit" @@ -41,6 +40,14 @@ type ClientConfig struct { RetryMaxDelay time.Duration } +// AppConfig identifies a GitHub App installation for client creation and caching. +// CredentialsSecretName may be left empty, in which case the factory falls back to its +// configured legacySecretName. +type AppConfig struct { + InstallationID int64 + CredentialsSecretName string +} + type RateLimitedError struct { ResetTime time.Time } @@ -79,12 +86,13 @@ type ClientInfo struct { // Credentials are cached per secret name and clients are cached per organization (cacheKey). // Rate limit state is shared per GitHub App ID so all installations of the same App share a quota bucket. type CachingGitHubClientFactory struct { - mu sync.RWMutex - clients map[string]*ClientInfo - config *ClientConfig - secretProvider SecretProviderFunc - credentials map[string]*AppCredentials - rateLimitStates map[int64]*github_primary_ratelimit.RateLimitState + mu sync.RWMutex + clients map[string]*ClientInfo + config *ClientConfig + secretProvider SecretProviderFunc + credentials map[string]*AppCredentials + rateLimitStates map[int64]*github_primary_ratelimit.RateLimitState + legacySecretName string } // AppCredentials holds parsed GitHub App credentials @@ -99,30 +107,36 @@ type SecretProviderFunc = func(ctx context.Context, secretName string) (*v1.Secr // NewGitHubCachingClientFactory creates a new client cache with the given configuration. The necessary GitHub App // credentials are fetched lazily via the given SecretProviderFunc upon first client creation for each secret. -func NewGitHubCachingClientFactory(config *ClientConfig, providerFunc SecretProviderFunc) (*CachingGitHubClientFactory, error) { +// legacySecretName is the fallback credentials secret name used for Organizations that still rely on the +// deprecated GitHubAppInstallationId field rather than the new GitHubAppConfig. +func NewGitHubCachingClientFactory(config *ClientConfig, providerFunc SecretProviderFunc, legacySecretName string) (*CachingGitHubClientFactory, error) { if config == nil { config = DefaultClientConfig() } manager := &CachingGitHubClientFactory{ - clients: make(map[string]*ClientInfo), - credentials: make(map[string]*AppCredentials), - rateLimitStates: make(map[int64]*github_primary_ratelimit.RateLimitState), - config: config, - secretProvider: providerFunc, + clients: make(map[string]*ClientInfo), + credentials: make(map[string]*AppCredentials), + rateLimitStates: make(map[int64]*github_primary_ratelimit.RateLimitState), + config: config, + secretProvider: providerFunc, + legacySecretName: legacySecretName, } return manager, nil } -// GetClient retrieves or creates a GitHub client for the given organization (cacheKey). +// GetClient retrieves or creates a GitHub client for the given cacheKey and AppConfig. +// If app.CredentialsSecretName is empty the factory falls back to its legacySecretName. // If a cached client exists for the cacheKey with matching credentials, it is returned directly. // If the credentials secret changed, the old client is evicted and a new one is created. -func (m *CachingGitHubClientFactory) GetClient(ctx context.Context, cacheKey string, app v1alpha1.GitHubAppConfig) (GitHubClient, error) { - log := logf.FromContext(ctx, - "function", "GetClient", - ) +func (m *CachingGitHubClientFactory) GetClient(ctx context.Context, cacheKey string, app AppConfig) (GitHubClient, error) { + secretName := app.CredentialsSecretName + if secretName == "" { + secretName = m.legacySecretName + } + log := logf.FromContext(ctx, "function", "GetClient") - if c := m.getCachedClient(cacheKey, app.CredentialsSecretName); c != nil { + if c := m.getCachedClient(cacheKey, secretName); c != nil { return c, nil } @@ -132,7 +146,7 @@ func (m *CachingGitHubClientFactory) GetClient(ctx context.Context, cacheKey str // Double-check after acquiring write lock if info, exists := m.clients[cacheKey]; exists { - if info.SecretName == app.CredentialsSecretName { + if info.SecretName == secretName { return info.Client, nil } // Credentials secret changed – evict the stale client @@ -142,7 +156,7 @@ func (m *CachingGitHubClientFactory) GetClient(ctx context.Context, cacheKey str log.Info("Creating new GitHub client") - ghClient, err := m.createClient(ctx, app) + ghClient, err := m.createClient(ctx, app.InstallationID, secretName) if err != nil { return nil, fmt.Errorf("failed to create GitHub client for key %s: %w", cacheKey, err) } @@ -152,17 +166,17 @@ func (m *CachingGitHubClientFactory) GetClient(ctx context.Context, cacheKey str m.clients[cacheKey] = &ClientInfo{ Client: wrappedClient, - InstallationID: app.InstallationId, + InstallationID: app.InstallationID, CacheKey: cacheKey, - SecretName: app.CredentialsSecretName, + SecretName: secretName, } - log.Info("Successfully created and cached GitHub client", "installationID", app.InstallationId) + log.Info("Successfully created and cached GitHub client", "installationID", app.InstallationID) return wrappedClient, nil } // GetGitHubClientAndCheckRateLimit retrieves a GitHub client and verifies the remaining rate limit. -func (m *CachingGitHubClientFactory) GetGitHubClientAndCheckRateLimit(ctx context.Context, cacheKey string, app v1alpha1.GitHubAppConfig, rateLimitMinimum int) (GitHubClient, error) { +func (m *CachingGitHubClientFactory) GetGitHubClientAndCheckRateLimit(ctx context.Context, cacheKey string, app AppConfig, rateLimitMinimum int) (GitHubClient, error) { ghClient, err := m.GetClient(ctx, cacheKey, app) if err != nil { return nil, err @@ -194,16 +208,16 @@ func (m *CachingGitHubClientFactory) getCachedClient(cacheKey string, secretName } // createClient creates a new GitHub client with proper middleware setup -func (m *CachingGitHubClientFactory) createClient(ctx context.Context, app v1alpha1.GitHubAppConfig) (*github.Client, error) { +func (m *CachingGitHubClientFactory) createClient(ctx context.Context, installationID int64, secretName string) (*github.Client, error) { log := logf.FromContext(ctx) log.Info("Creating GitHub client with middleware stack") - creds, ok := m.credentials[app.CredentialsSecretName] + creds, ok := m.credentials[secretName] if !ok { // Fetch and parse the secret on first use - secret, err := m.secretProvider(ctx, app.CredentialsSecretName) + secret, err := m.secretProvider(ctx, secretName) if err != nil { - log.Error(err, "failed to get GitHub app credentials secret", "secretName", app.CredentialsSecretName) + log.Error(err, "failed to get GitHub app credentials secret", "secretName", secretName) return nil, err } if secret == nil { @@ -214,11 +228,11 @@ func (m *CachingGitHubClientFactory) createClient(ctx context.Context, app v1alp log.Error(err, "failed to prepare GitHub app credentials") return nil, err } - m.credentials[app.CredentialsSecretName] = parsedCreds + m.credentials[secretName] = parsedCreds creds = parsedCreds } - ghClient, err := m.buildClientWithMiddleware(app.InstallationId, creds) + ghClient, err := m.buildClientWithMiddleware(installationID, creds) if err != nil { log.Error(err, "failed to create GitHub client") return nil, err diff --git a/internal/reconciler/orgrec/rec_rulesets.go b/internal/reconciler/orgrec/rec_rulesets.go index b4cbb02..d90c77d 100644 --- a/internal/reconciler/orgrec/rec_rulesets.go +++ b/internal/reconciler/orgrec/rec_rulesets.go @@ -49,6 +49,12 @@ func (o *GitHubOrgReconciler) reconcileRulesetPresets(ctx context.Context) error log.Error(err, "unable to get ruleset preset") return fmt.Errorf("failed to get ruleset preset %s: %w", rulesetRef.Name, err) } + + // Strip spec fields for disabled beta features before any processing. + if !o.Features.EnableRequiredReviewersRules && rulesetPreset.Spec.Rules.PullRequest != nil { + rulesetPreset.Spec.Rules.PullRequest.RequiredReviewers = nil + } + rulesetPreset, err := reconciler.ResolveNamesToIDsInRuleset(ctx, o.GitHub.Client, o.GitHub.Resource, rulesetPreset) if err != nil { log.Error(err, "failed to resolve ruleset slugs to IDs") diff --git a/internal/reconciler/orgrec/reconciler.go b/internal/reconciler/orgrec/reconciler.go index 58238e8..2ccfcac 100644 --- a/internal/reconciler/orgrec/reconciler.go +++ b/internal/reconciler/orgrec/reconciler.go @@ -6,6 +6,7 @@ import ( githubv1alpha1 "github.com/Interhyp/git-hubby/api/v1alpha1" ac "github.com/Interhyp/git-hubby/api/v1alpha1/applyconfiguration/api/v1alpha1" "github.com/Interhyp/git-hubby/internal/conditions" + "github.com/Interhyp/git-hubby/internal/config" "github.com/Interhyp/git-hubby/internal/reconciler" "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/runtime" @@ -32,6 +33,7 @@ func OrganizationStillHasTeamsError() error { type GitHubOrgReconciler struct { Kubernetes reconciler.Kubernetes[*githubv1alpha1.Organization] GitHub reconciler.GitHub[string] + Features config.Features } func (o *GitHubOrgReconciler) GetAdditionalLogFields() []any { diff --git a/internal/reconciler/reconcilerfactory/factory.go b/internal/reconciler/reconcilerfactory/factory.go index 4f4d32a..0ed0db8 100644 --- a/internal/reconciler/reconcilerfactory/factory.go +++ b/internal/reconciler/reconcilerfactory/factory.go @@ -3,9 +3,10 @@ package reconcilerfactory import ( "context" "fmt" - "os" "github.com/Interhyp/git-hubby/api/v1alpha1" + "github.com/Interhyp/git-hubby/internal/config" + "github.com/Interhyp/git-hubby/internal/ghclient" "github.com/Interhyp/git-hubby/internal/reconciler" "github.com/Interhyp/git-hubby/internal/reconciler/orgrec" "github.com/Interhyp/git-hubby/internal/reconciler/reporec" @@ -20,9 +21,7 @@ type Factory struct { ClientManager reconciler.GitHubClientManager K8sClient client.Client SpreadingManager reconciler.SpreadManager - // LegacySecretName is the name of the credential secret used for Organizations that - // still rely on the deprecated GitHubAppInstallationId field without a GitHubAppConfig. - LegacySecretName string + Config config.Config } const ( @@ -61,13 +60,13 @@ func (f *Factory) CreateForOrg(ctx context.Context, namespacedOrgName types.Name return nil, requiresSpreadErr } - appConfig, err := org.ResolveGitHubAppConfig(f.LegacySecretName) + appConfig, err := orgAppConfig(&org) if err != nil { logPkg.FromContext(ctx).Error(err, "unable to resolve GitHub App config for Organization") return nil, err } - ghClient, err := f.ClientManager.GetGitHubClientAndCheckRateLimit(ctx, org.GetLogin(), *appConfig, orgRateLimitThreshold) + ghClient, err := f.ClientManager.GetGitHubClientAndCheckRateLimit(ctx, org.GetLogin(), appConfig, orgRateLimitThreshold) if err != nil { return nil, err } @@ -82,6 +81,7 @@ func (f *Factory) CreateForOrg(ctx context.Context, namespacedOrgName types.Name Client: ghClient, Resource: org.GetLogin(), }, + Features: f.Config.Features, }, }, nil } @@ -115,13 +115,13 @@ func (f *Factory) CreateForRepo(ctx context.Context, repoName types.NamespacedNa return nil, err } - appConfig, err := org.ResolveGitHubAppConfig(f.LegacySecretName) + appConfig, err := orgAppConfig(&org) if err != nil { log.Error(err, "unable to resolve GitHub App config for Organization", "organization", org.GetLogin()) return nil, err } - ghClient, err := f.ClientManager.GetGitHubClientAndCheckRateLimit(ctx, org.GetLogin(), *appConfig, repoRateLimitThreshold) + ghClient, err := f.ClientManager.GetGitHubClientAndCheckRateLimit(ctx, org.GetLogin(), appConfig, repoRateLimitThreshold) if err != nil { return nil, err } @@ -139,7 +139,8 @@ func (f *Factory) CreateForRepo(ctx context.Context, repoName types.NamespacedNa Name: repo.Spec.Name, }, }, - FinalizeMode: reconciler.RepositoryFinalizerMode(os.Getenv("REPOSITORY_FINALIZER_MODE")), + FinalizeMode: reconciler.RepositoryFinalizerMode(f.Config.RepositoryFinalizerMode), + Features: f.Config.Features, }, }, nil } @@ -190,6 +191,7 @@ func (f *Factory) CreateForTeam(ctx context.Context, teamName types.NamespacedNa Client: f.K8sClient, Resource: &team, }, + MemberSuffix: f.Config.GitHubMemberSuffix, }, }, nil } @@ -299,14 +301,14 @@ func buildGitHubOrgsSlice(ctx context.Context, f *Factory, team v1alpha1.Team, r } var githubOrgs []reconciler.GitHub[string] for _, org := range orgs { - appConfig, err := org.ResolveGitHubAppConfig(f.LegacySecretName) + appConfig, err := orgAppConfig(&org) if err != nil { log.Error(err, "unable to resolve GitHub App config for Organization", "organization", org.GetLogin()) return nil, err } - ghRepo, err := f.ClientManager.GetGitHubClientAndCheckRateLimit(ctx, org.GetLogin(), *appConfig, teamRateLimitThreshold) + ghRepo, err := f.ClientManager.GetGitHubClientAndCheckRateLimit(ctx, org.GetLogin(), appConfig, teamRateLimitThreshold) if err != nil { - log.Error(err, "unable to get github client for installationId", "organization", org.GetLogin(), "installationId", appConfig.InstallationId) + log.Error(err, "unable to get github client for installationId", "organization", org.GetLogin(), "installationId", appConfig.InstallationID) return nil, err } @@ -318,3 +320,17 @@ func buildGitHubOrgsSlice(ctx context.Context, f *Factory, team v1alpha1.Team, r return githubOrgs, nil } + +// orgAppConfig extracts the minimal AppConfig needed by the GitHub client manager from an Organization. +// CredentialsSecretName is left empty for legacy orgs (those using the deprecated GitHubAppInstallationId +// field); the client manager will substitute its own legacySecretName in that case. +func orgAppConfig(org *v1alpha1.Organization) (ghclient.AppConfig, error) { + installationID, err := org.GetGitHubAppInstallationID() + if err != nil { + return ghclient.AppConfig{}, err + } + return ghclient.AppConfig{ + InstallationID: installationID, + CredentialsSecretName: org.GetGitHubAppCredentialsSecretName(), + }, nil +} diff --git a/internal/reconciler/reconcilerfactory/factory_test.go b/internal/reconciler/reconcilerfactory/factory_test.go index 910c337..7746605 100644 --- a/internal/reconciler/reconcilerfactory/factory_test.go +++ b/internal/reconciler/reconcilerfactory/factory_test.go @@ -153,7 +153,7 @@ var _ = Describe("Factory", func() { It("should call GetGitHubClientAndCheckRateLimit with correct parameters", func() { Expect(err).NotTo(HaveOccurred()) Expect(mockClientMgr.lastOrgName).To(Equal(defaultOrgName)) - Expect(mockClientMgr.lastAppConfig.InstallationId).To(Equal(defaultAppID)) + Expect(mockClientMgr.lastAppConfig.InstallationID).To(Equal(defaultAppID)) Expect(mockClientMgr.lastRateLimit).To(Equal(orgRateLimitThreshold)) }) }) @@ -296,7 +296,7 @@ var _ = Describe("Factory", func() { It("should call GetGitHubClientAndCheckRateLimit with correct parameters", func() { Expect(err).NotTo(HaveOccurred()) Expect(mockClientMgr.lastOrgName).To(Equal(defaultOrgName)) - Expect(mockClientMgr.lastAppConfig.InstallationId).To(Equal(defaultAppID)) + Expect(mockClientMgr.lastAppConfig.InstallationID).To(Equal(defaultAppID)) Expect(mockClientMgr.lastRateLimit).To(Equal(repoRateLimitThreshold)) }) }) @@ -1813,11 +1813,11 @@ type mockGitHubClientManager struct { genericErr error callCount int lastOrgName string - lastAppConfig v1alpha1.GitHubAppConfig + lastAppConfig ghclient.AppConfig lastRateLimit int } -func (m *mockGitHubClientManager) GetGitHubClientAndCheckRateLimit(_ context.Context, orgName string, app v1alpha1.GitHubAppConfig, rateLimitMinimum int) (ghclient.GitHubClient, error) { +func (m *mockGitHubClientManager) GetGitHubClientAndCheckRateLimit(_ context.Context, orgName string, app ghclient.AppConfig, rateLimitMinimum int) (ghclient.GitHubClient, error) { m.callCount++ m.lastOrgName = orgName m.lastAppConfig = app diff --git a/internal/reconciler/reporec/rec_rulesets.go b/internal/reconciler/reporec/rec_rulesets.go index ad62d4b..8afebf3 100644 --- a/internal/reconciler/reporec/rec_rulesets.go +++ b/internal/reconciler/reporec/rec_rulesets.go @@ -64,6 +64,11 @@ func (r *GitHubRepoReconciler) reconcileRuleSets(ctx context.Context) error { continue } + // Strip spec fields for disabled beta features before any processing. + if !r.Features.EnableRequiredReviewersRules && rulesetPreset.Spec.Rules.PullRequest != nil { + rulesetPreset.Spec.Rules.PullRequest.RequiredReviewers = nil + } + rulesetPreset, err := reconciler.ResolveNamesToIDsInRuleset(ctx, r.GitHub.Client, r.GitHub.Resource.Owner, rulesetPreset) if err != nil { log.Error(err, "failed to resolve ruleset slugs to IDs") diff --git a/internal/reconciler/reporec/reconciler.go b/internal/reconciler/reporec/reconciler.go index a96e1e2..75fc811 100644 --- a/internal/reconciler/reporec/reconciler.go +++ b/internal/reconciler/reporec/reconciler.go @@ -9,6 +9,7 @@ import ( githubv1alpha1 "github.com/Interhyp/git-hubby/api/v1alpha1" ac "github.com/Interhyp/git-hubby/api/v1alpha1/applyconfiguration/api/v1alpha1" "github.com/Interhyp/git-hubby/internal/conditions" + "github.com/Interhyp/git-hubby/internal/config" "github.com/Interhyp/git-hubby/internal/reconciler" "github.com/google/go-github/v89/github" "k8s.io/apimachinery/pkg/labels" @@ -34,6 +35,7 @@ type GitHubRepoReconciler struct { GitHub reconciler.GitHub[GitHubRepoIdentifier] Kubernetes reconciler.Kubernetes[*githubv1alpha1.Repository] FinalizeMode reconciler.RepositoryFinalizerMode + Features config.Features } func (r *GitHubRepoReconciler) K8s() reconciler.Kubernetes[*githubv1alpha1.Repository] { diff --git a/internal/reconciler/spreading/spreading.go b/internal/reconciler/spreading/spreading.go index a05ecc0..cfc8041 100644 --- a/internal/reconciler/spreading/spreading.go +++ b/internal/reconciler/spreading/spreading.go @@ -4,8 +4,6 @@ import ( "context" "hash/fnv" "math/rand/v2" - "os" - "strconv" "time" "github.com/google/uuid" @@ -13,6 +11,15 @@ import ( logf "sigs.k8s.io/controller-runtime/pkg/log" ) +const ( + // DefaultSpreadPeriodMinutes is the default spread-period window in minutes. + // Matches the envDefault in internal/config.Config.SpreadPeriodMinutes. + DefaultSpreadPeriodMinutes = 5 + // DefaultSpreadIntervalMinutes is the default spread-interval window in minutes. + // Matches the envDefault in internal/config.Config.SpreadIntervalMinutes. + DefaultSpreadIntervalMinutes = 180 +) + // SpreadableResource defines the interface for resources that support startup spreading type SpreadableResource interface { metav1.Object @@ -45,6 +52,17 @@ type Manager struct { Config Config } +// Option is a functional option for configuring a spreading Manager. +type Option func(*Config) + +// WithEnabled overrides the Enabled field of the default manager config. +// Pass spreading.WithEnabled(features.EnableStartupSpreading) from cmd/main.go. +func WithEnabled(enabled bool) Option { + return func(c *Config) { + c.Enabled = enabled + } +} + // NewManager creates a new spreading manager func NewManager(config Config) *Manager { return &Manager{ @@ -52,14 +70,35 @@ func NewManager(config Config) *Manager { } } -// NewDefaultManager creates a manager with default configuration from environment variables -func NewDefaultManager() *Manager { +// WithSpreadPeriod overrides the SpreadPeriod field of the default manager config. +// Pass spreading.WithSpreadPeriod(cfg.SpreadPeriodMinutes) from cmd/main.go. +func WithSpreadPeriod(minutes int) Option { + return func(c *Config) { + c.SpreadPeriod = time.Duration(minutes) * time.Minute + } +} + +// WithSpreadInterval overrides the SpreadInterval field of the default manager config. +// Pass spreading.WithSpreadInterval(cfg.SpreadIntervalMinutes) from cmd/main.go. +func WithSpreadInterval(minutes int) Option { + return func(c *Config) { + c.SpreadInterval = time.Duration(minutes) * time.Minute + } +} + +// NewDefaultManager creates a manager with default configuration from environment variables. +// The Enabled flag and numeric tuning values are not read from env here; pass the relevant +// With* opts from cmd/main.go so that configuration ownership stays in internal/config. +func NewDefaultManager(opts ...Option) *Manager { config := Config{ SpreadSeed: uuid.New().String(), StartTime: time.Now(), - Enabled: getBoolEnv("ENABLE_STARTUP_SPREADING", true), - SpreadPeriod: getDurationEnv("STARTUP_SPREAD_PERIOD_MINUTES", 5) * time.Minute, - SpreadInterval: getDurationEnv("SPREAD_INTERVAL_MINUTES", 180) * time.Minute, + Enabled: true, + SpreadPeriod: DefaultSpreadPeriodMinutes * time.Minute, + SpreadInterval: DefaultSpreadIntervalMinutes * time.Minute, + } + for _, opt := range opts { + opt(&config) } return &Manager{Config: config} } @@ -168,29 +207,3 @@ func hashString(s string) int { _, _ = h.Write([]byte(s)) return int(h.Sum32()) } - -// getBoolEnv reads a boolean from environment variable with a default -func getBoolEnv(key string, defaultValue bool) bool { - value := os.Getenv(key) - if value == "" { - return defaultValue - } - parsed, err := strconv.ParseBool(value) - if err != nil { - return defaultValue - } - return parsed -} - -// getDurationEnv reads a duration in minutes from environment variable with a default -func getDurationEnv(key string, defaultMinutes int) time.Duration { - value := os.Getenv(key) - if value == "" { - return time.Duration(defaultMinutes) - } - parsed, err := strconv.Atoi(value) - if err != nil { - return time.Duration(defaultMinutes) - } - return time.Duration(parsed) -} diff --git a/internal/reconciler/spreading/spreading_test.go b/internal/reconciler/spreading/spreading_test.go index 4c49cdc..1afcdb9 100644 --- a/internal/reconciler/spreading/spreading_test.go +++ b/internal/reconciler/spreading/spreading_test.go @@ -2,7 +2,6 @@ package spreading import ( "context" - "os" "time" . "github.com/onsi/ginkgo/v2" @@ -74,30 +73,19 @@ var _ = Describe("Spreading", func() { Expect(manager.Config.StartTime).NotTo(BeZero()) }) - It("should respect ENABLE_STARTUP_SPREADING=false", func() { - _ = os.Setenv("ENABLE_STARTUP_SPREADING", "false") - defer func() { _ = os.Unsetenv("ENABLE_STARTUP_SPREADING") }() - - manager := NewDefaultManager() + It("should disable spreading when WithEnabled(false) is passed", func() { + manager := NewDefaultManager(WithEnabled(false)) Expect(manager.Config.Enabled).To(BeFalse()) }) - It("should respect STARTUP_SPREAD_PERIOD_MINUTES", func() { - _ = os.Setenv("STARTUP_SPREAD_PERIOD_MINUTES", "10") - defer func() { _ = os.Unsetenv("STARTUP_SPREAD_PERIOD_MINUTES") }() - - manager := NewDefaultManager() - + It("should set SpreadPeriod when WithSpreadPeriod is passed", func() { + manager := NewDefaultManager(WithSpreadPeriod(10)) Expect(manager.Config.SpreadPeriod).To(Equal(10 * time.Minute)) }) - It("should respect SPREAD_INTERVAL_MINUTES", func() { - _ = os.Setenv("SPREAD_INTERVAL_MINUTES", "120") - defer func() { _ = os.Unsetenv("SPREAD_INTERVAL_MINUTES") }() - - manager := NewDefaultManager() - + It("should set SpreadInterval when WithSpreadInterval is passed", func() { + manager := NewDefaultManager(WithSpreadInterval(120)) Expect(manager.Config.SpreadInterval).To(Equal(120 * time.Minute)) }) }) @@ -591,55 +579,32 @@ var _ = Describe("Spreading", func() { }) }) - Describe("getBoolEnv", func() { - AfterEach(func() { - _ = os.Unsetenv("TEST_BOOL") + Describe("WithEnabled option", func() { + It("should enable the manager when passed true", func() { + m := NewDefaultManager(WithEnabled(true)) + Expect(m.Config.Enabled).To(BeTrue()) }) - It("should return default when env var is not set", func() { - result := getBoolEnv("TEST_BOOL", true) - Expect(result).To(BeTrue()) + It("should disable the manager when passed false", func() { + m := NewDefaultManager(WithEnabled(false)) + Expect(m.Config.Enabled).To(BeFalse()) }) - It("should parse 'true' correctly", func() { - _ = os.Setenv("TEST_BOOL", "true") - result := getBoolEnv("TEST_BOOL", false) - Expect(result).To(BeTrue()) - }) - - It("should parse 'false' correctly", func() { - _ = os.Setenv("TEST_BOOL", "false") - result := getBoolEnv("TEST_BOOL", true) - Expect(result).To(BeFalse()) - }) - - It("should return default on invalid value", func() { - _ = os.Setenv("TEST_BOOL", "invalid") - result := getBoolEnv("TEST_BOOL", true) - Expect(result).To(BeTrue()) + It("default manager is enabled when no option is passed", func() { + m := NewDefaultManager() + Expect(m.Config.Enabled).To(BeTrue()) }) }) - Describe("getDurationEnv", func() { - AfterEach(func() { - _ = os.Unsetenv("TEST_DURATION") - }) - - It("should return default when env var is not set", func() { - result := getDurationEnv("TEST_DURATION", 10) - Expect(result).To(Equal(time.Duration(10))) - }) - - It("should parse valid duration correctly", func() { - _ = os.Setenv("TEST_DURATION", "20") - result := getDurationEnv("TEST_DURATION", 10) - Expect(result).To(Equal(time.Duration(20))) + Describe("WithSpreadPeriod and WithSpreadInterval defaults", func() { + It("default manager uses DefaultSpreadPeriodMinutes", func() { + m := NewDefaultManager() + Expect(m.Config.SpreadPeriod).To(Equal(DefaultSpreadPeriodMinutes * time.Minute)) }) - It("should return default on invalid value", func() { - _ = os.Setenv("TEST_DURATION", "invalid") - result := getDurationEnv("TEST_DURATION", 10) - Expect(result).To(Equal(time.Duration(10))) + It("default manager uses DefaultSpreadIntervalMinutes", func() { + m := NewDefaultManager() + Expect(m.Config.SpreadInterval).To(Equal(DefaultSpreadIntervalMinutes * time.Minute)) }) }) }) diff --git a/internal/reconciler/teamrec/rec_members.go b/internal/reconciler/teamrec/rec_members.go index 6993c8e..c5493fd 100644 --- a/internal/reconciler/teamrec/rec_members.go +++ b/internal/reconciler/teamrec/rec_members.go @@ -3,7 +3,6 @@ package teamrec import ( "context" "fmt" - "os" githubv1alpha1 "github.com/Interhyp/git-hubby/api/v1alpha1" "sigs.k8s.io/controller-runtime/pkg/client" @@ -17,7 +16,7 @@ func (t *GitHubTeamReconciler) reconcileTeamMembers(ctx context.Context) error { return nil // IDP teams manage members via the identity provider } - envMemberSuffix := os.Getenv("GITHUB_MEMBER_SUFFIX") + envMemberSuffix := t.MemberSuffix k8sOrgs := make(map[string]githubv1alpha1.Organization) if envMemberSuffix == "" { // need to fetch orgs to check for org specific memberSuffixes later diff --git a/internal/reconciler/teamrec/rec_members_test.go b/internal/reconciler/teamrec/rec_members_test.go index 28d8362..bdc9ae1 100644 --- a/internal/reconciler/teamrec/rec_members_test.go +++ b/internal/reconciler/teamrec/rec_members_test.go @@ -71,8 +71,7 @@ var _ = Describe("ReconcileTeamMembers", func() { WithStatusSubresource(team). Build() - // Unset GITHUB_MEMBER_SUFFIX for tests - os.Unsetenv("GITHUB_MEMBER_SUFFIX") + // MemberSuffix defaults to empty for all tests; override in specific contexts via rec.MemberSuffix. }) Context("when team is an IDP team", func() { @@ -661,6 +660,7 @@ var _ = Describe("ReconcileTeamMembers", func() { } rec = &GitHubTeamReconciler{ + MemberSuffix: "_env", Team: reconciler.GitHubTeamIdentifier{ Name: "test-team", Slug: new("test-team"), diff --git a/internal/reconciler/teamrec/reconciler.go b/internal/reconciler/teamrec/reconciler.go index 4a503e7..63ac996 100644 --- a/internal/reconciler/teamrec/reconciler.go +++ b/internal/reconciler/teamrec/reconciler.go @@ -19,6 +19,10 @@ import ( type GitHubTeamReconciler struct { Team reconciler.GitHubTeamIdentifier Kubernetes reconciler.Kubernetes[*githubv1alpha1.Team] + // MemberSuffix is the global suffix appended to team member usernames (e.g. "@acme.com"). + // It is superseded by the per-organization spec.memberSuffix field when set. + // Corresponds to the GITHUB_MEMBER_SUFFIX environment variable loaded via internal/config. + MemberSuffix string } func (t *GitHubTeamReconciler) K8s() reconciler.Kubernetes[*githubv1alpha1.Team] { diff --git a/internal/reconciler/types.go b/internal/reconciler/types.go index ea9635b..672e32d 100644 --- a/internal/reconciler/types.go +++ b/internal/reconciler/types.go @@ -3,7 +3,6 @@ package reconciler import ( "context" - "github.com/Interhyp/git-hubby/api/v1alpha1" "github.com/Interhyp/git-hubby/internal/conditions" "github.com/Interhyp/git-hubby/internal/ghclient" "github.com/Interhyp/git-hubby/internal/reconciler/spreading" @@ -17,7 +16,7 @@ import ( const FieldOwner = client.FieldOwner("git-hubby") type GitHubClientManager interface { - GetGitHubClientAndCheckRateLimit(ctx context.Context, cacheKey string, app v1alpha1.GitHubAppConfig, rateLimitMinimum int) (ghclient.GitHubClient, error) + GetGitHubClientAndCheckRateLimit(ctx context.Context, cacheKey string, app ghclient.AppConfig, rateLimitMinimum int) (ghclient.GitHubClient, error) } type SpreadManager interface { diff --git a/internal/utils/utils.go b/internal/utils/utils.go index ab441a3..9488eac 100644 --- a/internal/utils/utils.go +++ b/internal/utils/utils.go @@ -20,13 +20,3 @@ func WithEmptyDefault[T any](value []T) []T { } return value } - -func BoolPtrDiffer(a, b *bool) bool { - if a == nil && b == nil { - return false - } - if a == nil || b == nil { - return true - } - return *a != *b -} diff --git a/internal/webhook/v1alpha1/repository_webhook.go b/internal/webhook/v1alpha1/repository_webhook.go index 0c4c38f..72ac5f2 100644 --- a/internal/webhook/v1alpha1/repository_webhook.go +++ b/internal/webhook/v1alpha1/repository_webhook.go @@ -21,19 +21,18 @@ import ( var repositorylog = logf.Log.WithName("repository-resource") // SetupRepositoryWebhookWithManager registers the webhook for Repository in the manager. -func SetupRepositoryWebhookWithManager(mgr ctrl.Manager, clientManager GitHubClientManager, legacySecretName string) error { +func SetupRepositoryWebhookWithManager(mgr ctrl.Manager, clientManager GitHubClientManager) error { return ctrl.NewWebhookManagedBy(mgr, &githubv1alpha1.Repository{}). WithValidator(&RepositoryCustomValidator{ K8sClient: mgr.GetClient(), GitHubClientManager: clientManager, - LegacySecretName: legacySecretName, }). Complete() } // GitHubClientManager is the interface the repository webhook uses to obtain a GitHub client. type GitHubClientManager interface { - GetClient(ctx context.Context, cacheKey string, app githubv1alpha1.GitHubAppConfig) (ghclient.GitHubClient, error) + GetClient(ctx context.Context, cacheKey string, app ghclient.AppConfig) (ghclient.GitHubClient, error) } // TODO(user): change verbs to "verbs=create;update;delete" if you want to enable deletion validation. @@ -49,9 +48,6 @@ type RepositoryCustomValidator struct { // TODO fugly: find a way to validate without doing either k8s or github api calls K8sClient client.Client GitHubClientManager GitHubClientManager - // LegacySecretName is the credential secret name used when the Organization uses the - // deprecated GitHubAppInstallationId field instead of the new GitHubAppConfig. - LegacySecretName string } var _ admission.Validator[*githubv1alpha1.Repository] = &RepositoryCustomValidator{} @@ -96,12 +92,15 @@ func (v *RepositoryCustomValidator) validateRepository(ctx context.Context, repo return fmt.Errorf("failed to fetch organization during validation of repository %s: %w", repo.Name, err) } - appConfig, err := org.ResolveGitHubAppConfig(v.LegacySecretName) + installationID, err := org.GetGitHubAppInstallationID() if err != nil { - return fmt.Errorf("failed to resolve GitHub App config for organization %s during validation of repository %s: %w", org.GetLogin(), repo.Name, err) + return fmt.Errorf("failed to resolve GitHub App installation ID for organization %s during validation of repository %s: %w", org.GetLogin(), repo.Name, err) } - githubClient, err := v.GitHubClientManager.GetClient(ctx, org.GetLogin(), *appConfig) + githubClient, err := v.GitHubClientManager.GetClient(ctx, org.GetLogin(), ghclient.AppConfig{ + InstallationID: installationID, + CredentialsSecretName: org.GetGitHubAppCredentialsSecretName(), + }) if err != nil { return fmt.Errorf("failed to create GitHub client for organization %s during validation of repository %s: %w", org.GetLogin(), repo.Name, err) } diff --git a/internal/webhook/v1alpha1/repository_webhook_test.go b/internal/webhook/v1alpha1/repository_webhook_test.go index 29a437d..844798c 100644 --- a/internal/webhook/v1alpha1/repository_webhook_test.go +++ b/internal/webhook/v1alpha1/repository_webhook_test.go @@ -135,7 +135,6 @@ var _ = Describe("Repository Webhook", func() { validator = RepositoryCustomValidator{ K8sClient: mockK8s, GitHubClientManager: ghclientmock.NewGitHubMockClientFactory(mockClient), - LegacySecretName: "test-credentials", } Expect(validator).NotTo(BeNil(), "Expected validator to be initialized") diff --git a/internal/webhook/v1alpha1/webhook_suite_test.go b/internal/webhook/v1alpha1/webhook_suite_test.go index 3b3ec65..52f5698 100644 --- a/internal/webhook/v1alpha1/webhook_suite_test.go +++ b/internal/webhook/v1alpha1/webhook_suite_test.go @@ -99,7 +99,7 @@ var _ = BeforeSuite(func() { Expect(err).NotTo(HaveOccurred()) mockClient := ghclientmock.NewMockGitHubClientWrapper() - err = SetupRepositoryWebhookWithManager(mgr, ghclientmock.NewGitHubMockClientFactory(mockClient), "test-credentials") + err = SetupRepositoryWebhookWithManager(mgr, ghclientmock.NewGitHubMockClientFactory(mockClient)) Expect(err).NotTo(HaveOccurred()) // +kubebuilder:scaffold:webhook diff --git a/test/mock/ghclientmock/mock_factory.go b/test/mock/ghclientmock/mock_factory.go index 1db5c1a..8024b1d 100644 --- a/test/mock/ghclientmock/mock_factory.go +++ b/test/mock/ghclientmock/mock_factory.go @@ -6,7 +6,6 @@ import ( "errors" "sync" - "github.com/Interhyp/git-hubby/api/v1alpha1" "github.com/Interhyp/git-hubby/internal/ghclient" "github.com/google/go-github/v89/github" ) @@ -21,14 +20,14 @@ func NewGitHubMockClientFactory(mockClient *MockGitHubClientWrapper) *GitHubMock } } -func (m *GitHubMockClientFactory) GetClient(_ context.Context, _ string, _ v1alpha1.GitHubAppConfig) (ghclient.GitHubClient, error) { +func (m *GitHubMockClientFactory) GetClient(_ context.Context, _ string, _ ghclient.AppConfig) (ghclient.GitHubClient, error) { if m.mockClient == nil { return nil, errors.New("mock GitHub client not set") } return m.mockClient, nil } -func (m *GitHubMockClientFactory) GetGitHubClientAndCheckRateLimit(_ context.Context, _ string, _ v1alpha1.GitHubAppConfig, _ int) (ghclient.GitHubClient, error) { +func (m *GitHubMockClientFactory) GetGitHubClientAndCheckRateLimit(_ context.Context, _ string, _ ghclient.AppConfig, _ int) (ghclient.GitHubClient, error) { if m.mockClient == nil { return nil, errors.New("mock GitHub client not set") }