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/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/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/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/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/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/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..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) {
@@ -148,9 +134,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,
@@ -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)
@@ -358,12 +339,20 @@ 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)
}
- 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,
@@ -374,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
@@ -390,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)
@@ -413,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)
@@ -436,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/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/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/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/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/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/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/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))))
+ })
+ })
+ })
})
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")
}