diff --git a/api/v1alpha1/applyconfiguration/api/v1alpha1/organizationspec.go b/api/v1alpha1/applyconfiguration/api/v1alpha1/organizationspec.go index 7dbefc7..0586838 100644 --- a/api/v1alpha1/applyconfiguration/api/v1alpha1/organizationspec.go +++ b/api/v1alpha1/applyconfiguration/api/v1alpha1/organizationspec.go @@ -29,8 +29,15 @@ import ( // rulesets, code security settings, and Actions permissions. // See: https://docs.github.com/en/rest/orgs/orgs type OrganizationSpecApplyConfiguration struct { - // Name is the GitHub organization name (also known as the organization login). - // This is the unique identifier for the organization on GitHub. + // Login is the GitHub organization login (the unique, immutable identifier on GitHub). + // This field is optional for backwards compatibility. If not specified, the Name field + // will be used as both login and display name. + // It is recommended to explicitly set this field to clearly separate login from display name. + Login *string `json:"login,omitempty"` + // Name is the organization's display name shown on the GitHub profile. + // If Login is not specified, this field will also be used as the organization login + // for backwards compatibility. + // At least one of Login or Name must be specified. Name *string `json:"name,omitempty"` // GitHubAppInstallationId is the numeric ID of the GitHub App installation for this organization. // This is used to authenticate API requests to GitHub. You can find this ID in your GitHub App's @@ -55,6 +62,12 @@ type OrganizationSpecApplyConfiguration struct { // Description is a human-readable description of the organization. // This appears on the organization's GitHub profile page. Description *string `json:"description,omitempty"` + // Location is the organization's location (e.g., "Munich, Germany"). + // This appears on the organization's GitHub profile page. + Location *string `json:"location,omitempty"` + // Website is the organization's website URL. + // This appears on the organization's GitHub profile page as a clickable link. + Website *string `json:"website,omitempty"` // Plan indicates the GitHub plan tier for this organization (enterprise, team, or free). // Determines whether Enterprise-only features (e.g., custom properties, runner groups) are reconciled or skipped. Plan *string `json:"plan,omitempty"` @@ -66,6 +79,14 @@ func OrganizationSpec() *OrganizationSpecApplyConfiguration { return &OrganizationSpecApplyConfiguration{} } +// WithLogin sets the Login 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 Login field is set to the value of the last call. +func (b *OrganizationSpecApplyConfiguration) WithLogin(value string) *OrganizationSpecApplyConfiguration { + b.Login = &value + return b +} + // WithName sets the Name 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 Name field is set to the value of the last call. @@ -134,6 +155,22 @@ func (b *OrganizationSpecApplyConfiguration) WithDescription(value string) *Orga return b } +// WithLocation sets the Location 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 Location field is set to the value of the last call. +func (b *OrganizationSpecApplyConfiguration) WithLocation(value string) *OrganizationSpecApplyConfiguration { + b.Location = &value + return b +} + +// WithWebsite sets the Website 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 Website field is set to the value of the last call. +func (b *OrganizationSpecApplyConfiguration) WithWebsite(value string) *OrganizationSpecApplyConfiguration { + b.Website = &value + return b +} + // WithPlan sets the Plan 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 Plan field is set to the value of the last call. diff --git a/api/v1alpha1/organization_methods.go b/api/v1alpha1/organization_methods.go index 982bde7..fa5cdec 100644 --- a/api/v1alpha1/organization_methods.go +++ b/api/v1alpha1/organization_methods.go @@ -98,3 +98,41 @@ func (o *OrgCustomPropertyDefaultValue) GetValues() []string { } return o.Values } + +// GetLogin returns the effective login for this organization. +// If Login is explicitly set, it returns Login. Otherwise, it returns Name. +func (o *Organization) GetLogin() string { + if o == nil { + return "" + } + if o.Spec.Login != "" { + return o.Spec.Login + } + return o.Spec.Name +} + +// GetDisplayName returns the effective display name for this organization. +// If both Login and Name are set, Name is the display name. +// If only Login is set, Login is used as display name. +// If only Name is set, Name is used as both login and display name. +func (o *Organization) GetDisplayName() string { + if o == nil { + return "" + } + if o.Spec.Login != "" && o.Spec.Name != "" { + return o.Spec.Name + } + if o.Spec.Login != "" && o.Spec.Name == "" { + return o.Spec.Login + } + return o.Spec.Name +} + +// IsUsingLegacyNameField returns true if only the Name field is set (backwards compatibility mode). +// This is used to generate deprecation warnings. +func (o *Organization) IsUsingLegacyNameField() bool { + if o == nil { + return false + } + return o.Spec.Login == "" && o.Spec.Name != "" +} diff --git a/api/v1alpha1/organization_types.go b/api/v1alpha1/organization_types.go index 9e4b346..fbcc057 100644 --- a/api/v1alpha1/organization_types.go +++ b/api/v1alpha1/organization_types.go @@ -279,14 +279,23 @@ type OrganizationSpec struct { // The following markers will use OpenAPI v3 schema to validate the value // More info: https://book.kubebuilder.io/reference/markers/crd-validation.html - // Name is the GitHub organization name (also known as the organization login). - // This is the unique identifier for the organization on GitHub. + // Login is the GitHub organization login (the unique, immutable identifier on GitHub). + // This field is optional for backwards compatibility. If not specified, the Name field + // will be used as both login and display name. + // It is recommended to explicitly set this field to clearly separate login from display name. // +kubebuilder:validation:MinLength=1 - // +kubebuilder:validation:MaxLength=100 - // +kubebuilder:validation:Pattern=`^[a-zA-Z0-9][a-zA-Z0-9_.-]{0,99}$` - // +kubebuilder:validation:Required - // +kubebuilder:validation:Type=string - Name string `json:"name"` + // +kubebuilder:validation:MaxLength=39 + // +optional + Login string `json:"login,omitempty"` + + // Name is the organization's display name shown on the GitHub profile. + // If Login is not specified, this field will also be used as the organization login + // for backwards compatibility. + // At least one of Login or Name must be specified. + // +kubebuilder:validation:MinLength=1 + // +kubebuilder:validation:MaxLength=255 + // +optional + Name string `json:"name,omitempty"` // GitHubAppInstallationId is the numeric ID of the GitHub App installation for this organization. // This is used to authenticate API requests to GitHub. You can find this ID in your GitHub App's @@ -320,6 +329,18 @@ type OrganizationSpec struct { // This appears on the organization's GitHub profile page. Description string `json:"description"` + // Location is the organization's location (e.g., "Munich, Germany"). + // This appears on the organization's GitHub profile page. + // +kubebuilder:validation:MaxLength=100 + // +optional + Location string `json:"location,omitempty"` + + // Website is the organization's website URL. + // This appears on the organization's GitHub profile page as a clickable link. + // +kubebuilder:validation:MaxLength=255 + // +optional + Website string `json:"website,omitempty"` + // Plan indicates the GitHub plan tier for this organization (enterprise, team, or free). // Determines whether Enterprise-only features (e.g., custom properties, runner groups) are reconciled or skipped. // +kubebuilder:validation:Enum=enterprise;team;free diff --git a/config/crd/bases/github.interhyp.de_organizations.yaml b/config/crd/bases/github.interhyp.de_organizations.yaml index 3a1d0e2..ed33d20 100644 --- a/config/crd/bases/github.interhyp.de_organizations.yaml +++ b/config/crd/bases/github.interhyp.de_organizations.yaml @@ -325,13 +325,29 @@ spec: format: int64 minimum: 1 type: integer - name: + location: description: |- - Name is the GitHub organization name (also known as the organization login). - This is the unique identifier for the organization on GitHub. + Location is the organization's location (e.g., "Munich, Germany"). + This appears on the organization's GitHub profile page. maxLength: 100 + type: string + login: + description: |- + Login is the GitHub organization login (the unique, immutable identifier on GitHub). + This field is optional for backwards compatibility. If not specified, the Name field + will be used as both login and display name. + It is recommended to explicitly set this field to clearly separate login from display name. + maxLength: 39 + minLength: 1 + type: string + name: + description: |- + Name is the organization's display name shown on the GitHub profile. + If Login is not specified, this field will also be used as the organization login + for backwards compatibility. + At least one of Login or Name must be specified. + maxLength: 255 minLength: 1 - pattern: ^[a-zA-Z0-9][a-zA-Z0-9_.-]{0,99}$ type: string plan: default: enterprise @@ -365,12 +381,17 @@ spec: type: object x-kubernetes-map-type: atomic type: array + website: + description: |- + Website is the organization's website URL. + This appears on the organization's GitHub profile page as a clickable link. + maxLength: 255 + type: string required: - actionsSettings - codeSecurityConfigurations - description - githubAppInstallationId - - name type: object status: description: status defines the observed state of Organization diff --git a/config/samples/github_v1alpha1_organization.yaml b/config/samples/github_v1alpha1_organization.yaml index dbe7076..f1faaf9 100644 --- a/config/samples/github_v1alpha1_organization.yaml +++ b/config/samples/github_v1alpha1_organization.yaml @@ -6,7 +6,11 @@ metadata: app.kubernetes.io/managed-by: kustomize name: sample-org spec: + login: sample-org + name: "Sample Organization" description: "GitHub Sample Organization" + location: "Munich, Germany" + website: "https://example.com" githubAppCredentials: secretRef: name: sample-org-app-credentials diff --git a/docs/techdocs/crds.md b/docs/techdocs/crds.md index dac5e81..a045eba 100644 --- a/docs/techdocs/crds.md +++ b/docs/techdocs/crds.md @@ -592,13 +592,16 @@ _Appears in:_ | Field | Description | Default | Validation | | --- | --- | --- | --- | -| `name` _string_ | Name is the GitHub organization name (also known as the organization login).
This is the unique identifier for the organization on GitHub. | | MaxLength: 100
MinLength: 1
Pattern: `^[a-zA-Z0-9][a-zA-Z0-9_.-]\{0,99\}$`
Required: \{\}
Type: string
| +| `login` _string_ | Login is the GitHub organization login (the unique, immutable identifier on GitHub).
This field is optional for backwards compatibility. If not specified, the Name field
will be used as both login and display name.
It is recommended to explicitly set this field to clearly separate login from display name. | | MaxLength: 39
MinLength: 1
Optional: \{\}
| +| `name` _string_ | Name is the organization's display name shown on the GitHub profile.
If Login is not specified, this field will also be used as the organization login
for backwards compatibility.
At least one of Login or Name must be specified. | | MaxLength: 255
MinLength: 1
Optional: \{\}
| | `githubAppInstallationId` _integer_ | GitHubAppInstallationId is the numeric ID of the GitHub App installation for this organization.
This is used to authenticate API requests to GitHub. You can find this ID in your GitHub App's
installation settings or via the GitHub API. | | Minimum: 1
Required: \{\}
| | `customProperties` _[OrgCustomProperty](#orgcustomproperty) array_ | CustomProperties defines custom metadata properties that can be assigned to repositories in the organization.
These properties allow you to categorize and add structured metadata to your repositories.
See: https://docs.github.com/en/rest/orgs/custom-properties | | MaxItems: 100
| | `actionsSettings` _[ActionsSettings](#actionssettings)_ | ActionsSettings configures GitHub Actions permissions and behavior for the organization.
This includes which repositories can use Actions, which actions are allowed, and runner group configurations.
See: https://docs.github.com/en/rest/actions/permissions | | | | `codeSecurityConfigurations` _[AttachableCodeSecurityConfigurationRef](#attachablecodesecurityconfigurationref) array_ | CodeSecurityConfigurations lists code security configurations to create and optionally attach to repositories.
Each configuration defines security features like dependency scanning, secret scanning, and code scanning.
See: https://docs.github.com/en/rest/code-security/configurations | | | | `rulesetPresets` _[LocalObjectReference](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.34/#localobjectreference-v1-core) array_ | RulesetPresetList references RulesetPreset CRDs that define repository rulesets for this organization.
Rulesets enforce policies like branch protection, required reviews, and status checks.
See: https://docs.github.com/en/rest/orgs/rules | | | | `description` _string_ | Description is a human-readable description of the organization.
This appears on the organization's GitHub profile page. | | | +| `location` _string_ | Location is the organization's location (e.g., "Munich, Germany").
This appears on the organization's GitHub profile page. | | MaxLength: 100
Optional: \{\}
| +| `website` _string_ | Website is the organization's website URL.
This appears on the organization's GitHub profile page as a clickable link. | | MaxLength: 255
Optional: \{\}
| | `plan` _string_ | Plan indicates the GitHub plan tier for this organization (enterprise, team, or free).
Determines whether Enterprise-only features (e.g., custom properties, runner groups) are reconciled or skipped. | enterprise | Enum: [enterprise team free]
Optional: \{\}
| diff --git a/internal/mapper/github_org_mapper.go b/internal/mapper/github_org_mapper.go index b13da67..0ec118a 100644 --- a/internal/mapper/github_org_mapper.go +++ b/internal/mapper/github_org_mapper.go @@ -6,19 +6,46 @@ import ( ) func OrgToGithubOrg(organization *v1alpha1.Organization) *github.Organization { - return &github.Organization{ - Name: &organization.Spec.Name, + displayName := organization.GetDisplayName() + login := organization.GetLogin() + ghOrg := &github.Organization{ + Login: &login, + Name: &displayName, Description: &organization.Spec.Description, } + + if organization.Spec.Location != "" { + ghOrg.Location = &organization.Spec.Location + } + if organization.Spec.Website != "" { + ghOrg.Blog = &organization.Spec.Website + } + + return ghOrg } func OrgDiffers(org *v1alpha1.Organization, githubOrg github.Organization) bool { - if org.Spec.Name != githubOrg.GetName() { + expectedLogin := org.GetLogin() + if expectedLogin != githubOrg.GetLogin() { + return true + } + + expectedDisplayName := org.GetDisplayName() + if expectedDisplayName != githubOrg.GetName() { return true } + if org.Spec.Description != githubOrg.GetDescription() { return true } + if org.Spec.Location != githubOrg.GetLocation() { + return true + } + + if org.Spec.Website != githubOrg.GetBlog() { + return true + } + return false } diff --git a/internal/mapper/github_org_mapper_test.go b/internal/mapper/github_org_mapper_test.go index 8859fc7..7f517bb 100644 --- a/internal/mapper/github_org_mapper_test.go +++ b/internal/mapper/github_org_mapper_test.go @@ -11,8 +11,8 @@ import ( var _ = Describe("GitHub Org Mapper", func() { Describe("OrgToGithubOrg", func() { - Context("when converting an organization with all fields set", func() { - It("should successfully convert to GitHub organization", func() { + Context("when converting an organization with only name field (legacy mode)", func() { + It("should use name as both login and display name", func() { org := &v1alpha1.Organization{ ObjectMeta: metav1.ObjectMeta{ Name: "test-org", @@ -31,6 +31,47 @@ var _ = Describe("GitHub Org Mapper", func() { }) }) + Context("when converting an organization with both login and name", func() { + It("should use name as display name", func() { + org := &v1alpha1.Organization{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-org", + }, + Spec: v1alpha1.OrganizationSpec{ + Login: "my-org-login", + Name: "My Organization Display Name", + Description: "Test description", + }, + } + + githubOrg := OrgToGithubOrg(org) + + Expect(githubOrg).NotTo(BeNil()) + Expect(githubOrg.Name).To(Equal(new("My Organization Display Name"))) + Expect(githubOrg.Description).To(Equal(new("Test description"))) + }) + }) + + Context("when converting an organization with only login field", func() { + It("should use login as display name", func() { + org := &v1alpha1.Organization{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-org", + }, + Spec: v1alpha1.OrganizationSpec{ + Login: "my-org-login", + Description: "Test description", + }, + } + + githubOrg := OrgToGithubOrg(org) + + Expect(githubOrg).NotTo(BeNil()) + Expect(githubOrg.Name).To(Equal(new("my-org-login"))) + Expect(githubOrg.Description).To(Equal(new("Test description"))) + }) + }) + Context("when converting an organization with empty description", func() { It("should set empty description", func() { org := &v1alpha1.Organization{ @@ -51,48 +92,91 @@ var _ = Describe("GitHub Org Mapper", func() { }) }) - Context("when converting an organization with long description", func() { - It("should preserve the full description", func() { - longDesc := "This is a very long description that contains multiple sentences. " + - "It describes the organization's purpose, mission, and values. " + - "Organizations can have descriptions up to a certain character limit." + Context("when converting an organization with Location set", func() { + It("should set Location in GitHub organization", func() { + org := &v1alpha1.Organization{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-org", + }, + Spec: v1alpha1.OrganizationSpec{ + Name: "my-org", + Location: "Munich, Germany", + Description: "Test description", + }, + } + + githubOrg := OrgToGithubOrg(org) + Expect(githubOrg).NotTo(BeNil()) + Expect(githubOrg.Location).To(Equal(new("Munich, Germany"))) + }) + }) + + Context("when converting an organization with Website set", func() { + It("should set Blog in GitHub organization", func() { org := &v1alpha1.Organization{ ObjectMeta: metav1.ObjectMeta{ Name: "test-org", }, Spec: v1alpha1.OrganizationSpec{ Name: "my-org", - Description: longDesc, + Website: "https://example.com", + Description: "Test description", }, } githubOrg := OrgToGithubOrg(org) Expect(githubOrg).NotTo(BeNil()) - Expect(githubOrg.Name).To(Equal(new("my-org"))) - Expect(githubOrg.Description).To(Equal(new(longDesc))) + Expect(githubOrg.Blog).To(Equal(new("https://example.com"))) }) }) - Context("when converting an organization with special characters in description", func() { - It("should preserve special characters", func() { - specialDesc := "Org with special chars: @#$%^&*()_+-=[]{}|;:',.<>?/~`" + Context("when converting an organization with all fields set", func() { + It("should set all fields correctly", func() { + org := &v1alpha1.Organization{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-org", + }, + Spec: v1alpha1.OrganizationSpec{ + Login: "my-org-login", + Name: "My Org Display Name", + Location: "Munich, Germany", + Website: "https://example.com", + Description: "Test description", + }, + } + + githubOrg := OrgToGithubOrg(org) + + Expect(githubOrg).NotTo(BeNil()) + Expect(githubOrg.Name).To(Equal(new("My Org Display Name"))) + Expect(githubOrg.Location).To(Equal(new("Munich, Germany"))) + Expect(githubOrg.Blog).To(Equal(new("https://example.com"))) + Expect(githubOrg.Description).To(Equal(new("Test description"))) + }) + }) + Context("when converting an organization with empty optional fields", func() { + It("should not set Location and Blog when empty", func() { org := &v1alpha1.Organization{ ObjectMeta: metav1.ObjectMeta{ Name: "test-org", }, Spec: v1alpha1.OrganizationSpec{ Name: "my-org", - Description: specialDesc, + Location: "", + Website: "", + Description: "Test description", }, } githubOrg := OrgToGithubOrg(org) Expect(githubOrg).NotTo(BeNil()) - Expect(githubOrg.Description).To(Equal(new(specialDesc))) + Expect(githubOrg.Name).To(Equal(new("my-org"))) + Expect(githubOrg.Location).To(BeNil()) + Expect(githubOrg.Blog).To(BeNil()) }) }) }) @@ -112,9 +196,10 @@ var _ = Describe("GitHub Org Mapper", func() { } }) - Context("when organizations match exactly", func() { + Context("when organizations match exactly (legacy mode)", func() { It("should return false", func() { githubOrg := github.Organization{ + Login: new("my-org"), Name: new("my-org"), Description: new("Test organization"), } @@ -125,10 +210,11 @@ var _ = Describe("GitHub Org Mapper", func() { }) }) - Context("when name differs", func() { + Context("when login differs", func() { It("should return true", func() { githubOrg := github.Organization{ - Name: new("different-org"), + Login: new("different-org"), + Name: new("my-org"), Description: new("Test organization"), } @@ -138,11 +224,12 @@ var _ = Describe("GitHub Org Mapper", func() { }) }) - Context("when description differs", func() { + Context("when display name differs", func() { It("should return true", func() { githubOrg := github.Organization{ - Name: new("my-org"), - Description: new("Different description"), + Login: new("my-org"), + Name: new("Different Name"), + Description: new("Test organization"), } differs := OrgDiffers(org, githubOrg) @@ -151,10 +238,11 @@ var _ = Describe("GitHub Org Mapper", func() { }) }) - Context("when both name and description differ", func() { + Context("when description differs", func() { It("should return true", func() { githubOrg := github.Organization{ - Name: new("different-org"), + Login: new("my-org"), + Name: new("my-org"), Description: new("Different description"), } @@ -164,10 +252,11 @@ var _ = Describe("GitHub Org Mapper", func() { }) }) - Context("when GitHub organization has nil name", func() { + Context("when GitHub organization has nil login", func() { It("should return true", func() { githubOrg := github.Organization{ - Name: nil, + Login: nil, + Name: new("my-org"), Description: new("Test organization"), } @@ -177,25 +266,42 @@ var _ = Describe("GitHub Org Mapper", func() { }) }) - Context("when GitHub organization has nil description", func() { - It("should return true", func() { + Context("when Location differs", func() { + It("should return true when K8s has Location but GitHub does not", func() { + org.Spec.Location = "Munich, Germany" githubOrg := github.Organization{ + Login: new("my-org"), Name: new("my-org"), - Description: nil, + Description: new("Test organization"), + Location: nil, } differs := OrgDiffers(org, githubOrg) Expect(differs).To(BeTrue()) }) - }) - Context("when both organizations have empty descriptions", func() { - It("should return false", func() { - org.Spec.Description = "" + It("should return true when Location values differ", func() { + org.Spec.Location = "Munich, Germany" + githubOrg := github.Organization{ + Login: new("my-org"), + Name: new("my-org"), + Description: new("Test organization"), + Location: new("Berlin, Germany"), + } + + differs := OrgDiffers(org, githubOrg) + + Expect(differs).To(BeTrue()) + }) + + It("should return false when Location values match", func() { + org.Spec.Location = "Munich, Germany" githubOrg := github.Organization{ + Login: new("my-org"), Name: new("my-org"), - Description: new(""), + Description: new("Test organization"), + Location: new("Munich, Germany"), } differs := OrgDiffers(org, githubOrg) @@ -204,38 +310,59 @@ var _ = Describe("GitHub Org Mapper", func() { }) }) - Context("when K8s description is empty but GitHub has description", func() { - It("should return true", func() { - org.Spec.Description = "" + Context("when Website differs", func() { + It("should return true when K8s has Website but GitHub does not", func() { + org.Spec.Website = "https://example.com" githubOrg := github.Organization{ + Login: new("my-org"), Name: new("my-org"), - Description: new("Some description"), + Description: new("Test organization"), + Blog: nil, } differs := OrgDiffers(org, githubOrg) Expect(differs).To(BeTrue()) }) - }) - Context("when K8s has description but GitHub description is empty", func() { - It("should return true", func() { + It("should return false when Website values match", func() { + org.Spec.Website = "https://example.com" githubOrg := github.Organization{ + Login: new("my-org"), Name: new("my-org"), - Description: new(""), + Description: new("Test organization"), + Blog: new("https://example.com"), } differs := OrgDiffers(org, githubOrg) - Expect(differs).To(BeTrue()) + Expect(differs).To(BeFalse()) }) }) - Context("when checking whitespace differences", func() { - It("should detect trailing whitespace differences", func() { + Context("when using explicit login and name", func() { + BeforeEach(func() { + org.Spec.Login = "my-org-login" + org.Spec.Name = "My Organization Display Name" + }) + + It("should compare login and display name separately", func() { githubOrg := github.Organization{ - Name: new("my-org"), - Description: new("Test organization "), + Login: new("my-org-login"), + Name: new("My Organization Display Name"), + Description: new("Test organization"), + } + + differs := OrgDiffers(org, githubOrg) + + Expect(differs).To(BeFalse()) + }) + + It("should return true when login matches but display name differs", func() { + githubOrg := github.Organization{ + Login: new("my-org-login"), + Name: new("Different Display Name"), + Description: new("Test organization"), } differs := OrgDiffers(org, githubOrg) @@ -243,10 +370,11 @@ var _ = Describe("GitHub Org Mapper", func() { Expect(differs).To(BeTrue()) }) - It("should detect leading whitespace differences", func() { + It("should return true when display name matches but login differs", func() { githubOrg := github.Organization{ - Name: new("my-org"), - Description: new(" Test organization"), + Login: new("different-login"), + Name: new("My Organization Display Name"), + Description: new("Test organization"), } differs := OrgDiffers(org, githubOrg) @@ -254,5 +382,139 @@ var _ = Describe("GitHub Org Mapper", func() { Expect(differs).To(BeTrue()) }) }) + + Context("when all fields match", func() { + It("should return false", func() { + org.Spec.Login = "my-org-login" + org.Spec.Name = "My Org Display Name" + org.Spec.Location = "Munich, Germany" + org.Spec.Website = "https://example.com" + githubOrg := github.Organization{ + Login: new("my-org-login"), + Name: new("My Org Display Name"), + Description: new("Test organization"), + Location: new("Munich, Germany"), + Blog: new("https://example.com"), + } + + differs := OrgDiffers(org, githubOrg) + + Expect(differs).To(BeFalse()) + }) + }) + }) + + Describe("Organization Helper Methods", func() { + Describe("GetLogin", func() { + It("should return Login when explicitly set", func() { + org := &v1alpha1.Organization{ + Spec: v1alpha1.OrganizationSpec{ + Login: "my-login", + Name: "My Display Name", + }, + } + + Expect(org.GetLogin()).To(Equal("my-login")) + }) + + It("should return Name when Login is not set (legacy mode)", func() { + org := &v1alpha1.Organization{ + Spec: v1alpha1.OrganizationSpec{ + Name: "my-org", + }, + } + + Expect(org.GetLogin()).To(Equal("my-org")) + }) + + It("should return empty string when nil", func() { + var org *v1alpha1.Organization + Expect(org.GetLogin()).To(Equal("")) + }) + }) + + Describe("GetDisplayName", func() { + It("should return Name when both Login and Name are set", func() { + org := &v1alpha1.Organization{ + Spec: v1alpha1.OrganizationSpec{ + Login: "my-login", + Name: "My Display Name", + }, + } + + Expect(org.GetDisplayName()).To(Equal("My Display Name")) + }) + + It("should return Login when only Login is set", func() { + org := &v1alpha1.Organization{ + Spec: v1alpha1.OrganizationSpec{ + Login: "my-login", + }, + } + + Expect(org.GetDisplayName()).To(Equal("my-login")) + }) + + It("should return Name when only Name is set (legacy mode)", func() { + org := &v1alpha1.Organization{ + Spec: v1alpha1.OrganizationSpec{ + Name: "my-org", + }, + } + + Expect(org.GetDisplayName()).To(Equal("my-org")) + }) + + It("should return empty string when nil", func() { + var org *v1alpha1.Organization + Expect(org.GetDisplayName()).To(Equal("")) + }) + }) + + Describe("IsUsingLegacyNameField", func() { + It("should return true when only Name is set", func() { + org := &v1alpha1.Organization{ + Spec: v1alpha1.OrganizationSpec{ + Name: "my-org", + }, + } + + Expect(org.IsUsingLegacyNameField()).To(BeTrue()) + }) + + It("should return false when Login is set", func() { + org := &v1alpha1.Organization{ + Spec: v1alpha1.OrganizationSpec{ + Login: "my-login", + Name: "My Display Name", + }, + } + + Expect(org.IsUsingLegacyNameField()).To(BeFalse()) + }) + + It("should return false when only Login is set", func() { + org := &v1alpha1.Organization{ + Spec: v1alpha1.OrganizationSpec{ + Login: "my-login", + }, + } + + Expect(org.IsUsingLegacyNameField()).To(BeFalse()) + }) + + It("should return false when nil", func() { + var org *v1alpha1.Organization + Expect(org.IsUsingLegacyNameField()).To(BeFalse()) + }) + + It("should return false when both are empty", func() { + org := &v1alpha1.Organization{ + Spec: v1alpha1.OrganizationSpec{}, + } + + Expect(org.IsUsingLegacyNameField()).To(BeFalse()) + }) + }) }) }) diff --git a/internal/reconciler/orgrec/rec_org_test.go b/internal/reconciler/orgrec/rec_org_test.go index f529f75..a98a82a 100644 --- a/internal/reconciler/orgrec/rec_org_test.go +++ b/internal/reconciler/orgrec/rec_org_test.go @@ -438,6 +438,81 @@ var _ = Describe("ReconcileOrganization", func() { }) }) + Context("when Location needs to be updated", func() { + BeforeEach(func() { + org.Spec.Location = "Munich, Germany" + currentGHOrg = &github.Organization{ + Name: new("test-org"), + Description: new("Test Organization"), + Login: new("test-org"), + Location: nil, + } + }) + + It("should trigger update to set Location", func() { + Expect(err).NotTo(HaveOccurred()) + Expect(editOrgCalled).To(BeTrue()) + Expect(editedOrg.GetLocation()).To(Equal("Munich, Germany")) + }) + }) + + Context("when Website needs to be updated", func() { + BeforeEach(func() { + org.Spec.Website = "https://example.com" + currentGHOrg = &github.Organization{ + Name: new("test-org"), + Description: new("Test Organization"), + Login: new("test-org"), + Blog: nil, + } + }) + + It("should trigger update to set Website (Blog)", func() { + Expect(err).NotTo(HaveOccurred()) + Expect(editOrgCalled).To(BeTrue()) + Expect(editedOrg.GetBlog()).To(Equal("https://example.com")) + }) + }) + + Context("when display name needs to be updated (using login and name)", func() { + BeforeEach(func() { + org.Spec.Login = "test-org" + org.Spec.Name = "My Organization Display Name" + currentGHOrg = &github.Organization{ + Name: new("test-org"), + Description: new("Test Organization"), + Login: new("test-org"), + } + }) + + It("should trigger update to set display name", func() { + Expect(err).NotTo(HaveOccurred()) + Expect(editOrgCalled).To(BeTrue()) + Expect(editedOrg.GetName()).To(Equal("My Organization Display Name")) + }) + }) + + Context("when all new profile fields match", func() { + BeforeEach(func() { + org.Spec.Login = "test-org" + org.Spec.Name = "My Organization" + org.Spec.Location = "Munich, Germany" + org.Spec.Website = "https://example.com" + currentGHOrg = &github.Organization{ + Name: new("My Organization"), + Description: new("Test Organization"), + Login: new("test-org"), + Location: new("Munich, Germany"), + Blog: new("https://example.com"), + } + }) + + It("should not trigger update", func() { + Expect(err).NotTo(HaveOccurred()) + Expect(editOrgCalled).To(BeFalse()) + }) + }) + Context("when only whitespace changes in description", func() { BeforeEach(func() { org.Spec.Description = "Test Organization" @@ -515,11 +590,9 @@ var _ = Describe("ReconcileOrganization", func() { Name: new("test-org"), Description: new("Test Organization"), Login: new("test-org"), - // Additional fields that aren't in our spec - Company: new("Test Company"), - Blog: new("https://example.com"), - Location: new("Test Location"), - Email: new("test@example.com"), + // Additional fields that aren't in our spec (Company and Email are truly unmanaged) + Company: new("Test Company"), + Email: new("test@example.com"), } }) diff --git a/internal/reconciler/orgrec/reconciler.go b/internal/reconciler/orgrec/reconciler.go index b3705f9..58238e8 100644 --- a/internal/reconciler/orgrec/reconciler.go +++ b/internal/reconciler/orgrec/reconciler.go @@ -42,7 +42,7 @@ func (o *GitHubOrgReconciler) GetAdditionalLogFields() []any { func (o *GitHubOrgReconciler) GetAdditionalLabels() labels.Set { return labels.Set{ - "git-hubby.interhyp.de/organization": o.Kubernetes.Resource.Spec.Name, + "git-hubby.interhyp.de/organization": o.Kubernetes.Resource.GetLogin(), } } diff --git a/internal/reconciler/reconcilerfactory/factory.go b/internal/reconciler/reconcilerfactory/factory.go index 926b4c4..3c327e3 100644 --- a/internal/reconciler/reconcilerfactory/factory.go +++ b/internal/reconciler/reconcilerfactory/factory.go @@ -31,15 +31,24 @@ const ( // CreateForOrg creates a reconciler.ReconciliationExecutor for a v1alpha1.Organization // If both the returned Executor and the error are nil, the Organization K8s resource was not found and no reconciliation is necessary. func (f *Factory) CreateForOrg(ctx context.Context, namespacedOrgName types.NamespacedName) (*reconciler.ReconciliationExecutor[*v1alpha1.Organization], error) { + log := logPkg.FromContext(ctx) + var org v1alpha1.Organization if err := f.K8sClient.Get(ctx, namespacedOrgName, &org); err != nil { notFoundIgnored := client.IgnoreNotFound(err) if notFoundIgnored != nil { - logPkg.FromContext(ctx).Error(notFoundIgnored, "unable to fetch Organization") + log.Error(notFoundIgnored, "unable to fetch Organization") } return nil, notFoundIgnored } + // Log deprecation warning if using legacy name-only mode + if org.IsUsingLegacyNameField() { + log.Info("DEPRECATED: Organization uses 'name' field without explicit 'login' field. Consider setting 'login' to separate login from display name", + "organization", org.Name, + "effectiveLogin", org.GetLogin()) + } + subResourceGenerations, err := f.fetchSubResourceGenerationsForOrg(ctx, org) if err != nil { return nil, err @@ -49,7 +58,7 @@ func (f *Factory) CreateForOrg(ctx context.Context, namespacedOrgName types.Name return nil, requiresSpreadErr } - ghClient, err := f.ClientManager.GetGitHubClientAndCheckRateLimit(ctx, org.Spec.Name, org.Spec.GitHubAppInstallationId, orgRateLimitThreshold) + ghClient, err := f.ClientManager.GetGitHubClientAndCheckRateLimit(ctx, org.GetLogin(), org.Spec.GitHubAppInstallationId, orgRateLimitThreshold) if err != nil { return nil, err } @@ -62,7 +71,7 @@ func (f *Factory) CreateForOrg(ctx context.Context, namespacedOrgName types.Name }, GitHub: reconciler.GitHub[string]{ Client: ghClient, - Resource: org.Spec.Name, + Resource: org.GetLogin(), }, }, }, nil @@ -97,7 +106,7 @@ func (f *Factory) CreateForRepo(ctx context.Context, repoName types.NamespacedNa return nil, err } - ghClient, err := f.ClientManager.GetGitHubClientAndCheckRateLimit(ctx, org.Spec.Name, org.Spec.GitHubAppInstallationId, repoRateLimitThreshold) + ghClient, err := f.ClientManager.GetGitHubClientAndCheckRateLimit(ctx, org.GetLogin(), org.Spec.GitHubAppInstallationId, repoRateLimitThreshold) if err != nil { return nil, err } @@ -111,7 +120,7 @@ func (f *Factory) CreateForRepo(ctx context.Context, repoName types.NamespacedNa GitHub: reconciler.GitHub[reporec.GitHubRepoIdentifier]{ Client: ghClient, Resource: reporec.GitHubRepoIdentifier{ - Owner: org.Spec.Name, + Owner: org.GetLogin(), Name: repo.Spec.Name, }, }, @@ -275,15 +284,15 @@ func buildGitHubOrgsSlice(ctx context.Context, f *Factory, team v1alpha1.Team, r } var githubOrgs []reconciler.GitHub[string] for _, org := range orgs { - ghRepo, err := f.ClientManager.GetGitHubClientAndCheckRateLimit(ctx, org.Spec.Name, org.Spec.GitHubAppInstallationId, teamRateLimitThreshold) + ghRepo, err := f.ClientManager.GetGitHubClientAndCheckRateLimit(ctx, org.GetLogin(), org.Spec.GitHubAppInstallationId, teamRateLimitThreshold) if err != nil { - log.Error(err, "unable to get github client for installationId", "organization", org.Spec.Name, "installationId", org.Spec.GitHubAppInstallationId) + log.Error(err, "unable to get github client for installationId", "organization", org.GetLogin(), "installationId", org.Spec.GitHubAppInstallationId) return nil, err } githubOrgs = append(githubOrgs, reconciler.GitHub[string]{ Client: ghRepo, - Resource: org.Spec.Name, + Resource: org.GetLogin(), }) } diff --git a/internal/webhook/v1alpha1/organization_webhook.go b/internal/webhook/v1alpha1/organization_webhook.go index 5dc12d4..a665f7a 100644 --- a/internal/webhook/v1alpha1/organization_webhook.go +++ b/internal/webhook/v1alpha1/organization_webhook.go @@ -78,8 +78,16 @@ func (v *OrganizationCustomValidator) ValidateUpdate(_ context.Context, _ *githu func validateOrganization(organization *githubv1alpha1.Organization) error { var allErrs field.ErrorList + // Validate that at least one of login or name is set + if organization.Spec.Login == "" && organization.Spec.Name == "" { + allErrs = append(allErrs, field.Required( + field.NewPath("spec"), + "either 'login' or 'name' must be specified", + )) + } + customPropertiesField := field.NewPath("spec").Child("customProperties") - allErrs = validateCustomProperties(organization.Spec.CustomProperties, customPropertiesField) + allErrs = append(allErrs, validateCustomProperties(organization.Spec.CustomProperties, customPropertiesField)...) allErrs = append(allErrs, validatePlanFeatureCombinations(organization)...) diff --git a/internal/webhook/v1alpha1/repository_webhook.go b/internal/webhook/v1alpha1/repository_webhook.go index ab797c8..7a31209 100644 --- a/internal/webhook/v1alpha1/repository_webhook.go +++ b/internal/webhook/v1alpha1/repository_webhook.go @@ -107,13 +107,13 @@ func (v *RepositoryCustomValidator) validateRepository(ctx context.Context, repo if err := v.K8sClient.Get(ctx, client.ObjectKey{Name: repo.Spec.OrganizationRef.Name, Namespace: repo.Namespace}, &org); err != nil { return fmt.Errorf("failed to fetch organization during validation of repository %s: %w", repo.Name, err) } - githubClient, err := v.GitHubClientManager.GetClient(ctx, org.Spec.Name, org.Spec.GitHubAppInstallationId) + githubClient, err := v.GitHubClientManager.GetClient(ctx, org.GetLogin(), org.Spec.GitHubAppInstallationId) if err != nil { - return fmt.Errorf("failed to create GitHub client for organization %s during validation of repository %s: %w", org.Spec.Name, repo.Name, err) + return fmt.Errorf("failed to create GitHub client for organization %s during validation of repository %s: %w", org.GetLogin(), repo.Name, err) } - customPropertyDefinitions, err := githubClient.GetAllCustomPropertiesForOrganization(ctx, org.Spec.Name) + customPropertyDefinitions, err := githubClient.GetAllCustomPropertiesForOrganization(ctx, org.GetLogin()) if err != nil { - return fmt.Errorf("failed to fetch custom properties for GitHub organization %s during validation of repository %s: %w", org.Spec.Name, repo.Name, err) + return fmt.Errorf("failed to fetch custom properties for GitHub organization %s during validation of repository %s: %w", org.GetLogin(), repo.Name, err) } // TODO end of external requests