peribolos: Add Repository Fork Management Support
#583
Conversation
…gnment This refactoring separates GitHub Repos API fields into a dedicated RepoMetadata struct, following the same pattern as TeamMetadata/Team and Metadata/Config. Changes: - Create RepoMetadata struct with fields that map directly to GitHub's Update Repository API - Embed RepoMetadata in Repo struct - Keep Peribolos-specific fields (Previously, OnCreate) and fields managed by other APIs (Collaborators) in the Repo struct - Update all struct literals in main.go and tests to use the new embedded struct syntax This change is backwards compatible - the YAML/JSON serialization remains unchanged due to Go's struct embedding behavior.
Add the ability to manage repository forks in Peribolos, allowing users
to declare that a repository should be created as a fork of an upstream
repository.
Configuration changes (pkg/config/org/org.go):
- Add ForkFrom field to specify upstream repository (format: owner/repo)
- Add DefaultBranchOnly field to fork only the default branch
GitHub client changes (pkg/github/client.go):
- Add CreateForkInOrg() to create a fork in a specific organization
- Add ListForks() to list forks of a repository
- Add methods to RepositoryClient interface
Peribolos changes (cmd/peribolos/main.go):
- Add --fix-forks flag to enable fork management
- Add configureForks() function to create forks from upstream
- Update dump to include fork_from field for existing forks
Example configuration:
repos:
my-fork:
fork_from: kubernetes/kubernetes
default_branch_only: true
description: Our fork of Kubernetes
See https://docs.github.com/en/rest/repos/forks for API documentation.
✅ Deploy Preview for k8s-prow ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
k8s-ci-robot
added
the
area/peribolos
k8s-ci-robot
added
cncf-cla: yes
|
Hi @hoxhaeris. Thanks for your PR. I'm waiting for a github.com member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
k8s-ci-robot
added
the
size/XL
|
/cc @petr-muller |
hoxhaeris
changed the title
peribolos: Add Repository Fork Management Support
k8s-ci-robot
added
ok-to-test
k8s-ci-robot
added
the
lgtm
|
/hold |
k8s-ci-robot
added
the
do-not-merge/hold
|
I have reviewed this PR and would like to recommend it for addition to the codebase. The changes aligns well with existing patterns, have adequate test coverage, and are protected by the |
|
hi, the PR seems well written indeed, just wondering why we need the flag |
Mostly to have more granular control (although most users would want them both, |
petr-muller
left a comment
petr-muller
left a comment
There was a problem hiding this comment.
Just want to clarify some things, so I'm approving with a hold.
/hold
my-fork:
fork_from: kubernetes/kubernetes
default_branch_only: true
description: Our fork of **Kubernetes**
Is this snippet configuring a my-org/my-fork to exist, or my-org/kubernetes? IIUC it is the former, right?
cmd/peribolos/main_test.go
Outdated
| func boolPtr(b bool) *bool { | ||
| return &b | ||
| } | ||
|
|
||
| func strPtr(s string) *string { | ||
| return &s | ||
| } |
There was a problem hiding this comment.
Don't bother respinning the PR for this but there's generic ptr.To now: https://pkg.go.dev/k8s.io/utils/ptr#To (also useful in other codebases that import k8s utils ;) )
There was a problem hiding this comment.
Since I had to respin the PR for a few other fixes anyway, I went ahead and made this change as well. Done in 04ef97729
cmd/peribolos/main.go
Outdated
| // Note: GitHub may name the fork differently if there's a naming conflict | ||
| if createdName != repoName { | ||
| repoLogger.WithField("created_name", createdName).Warn("fork was created with a different name than expected") | ||
| } else { | ||
| repoLogger.Info("fork created successfully") | ||
| } |
There was a problem hiding this comment.
I'm not sure if I correctly understand how this is idempotent. We config a fork to exist in our subject org, after first run it is created under an arbitrary name. How do the subsequent runs avoid trying to recreate a fork if the existing one has an arbitrary name not present in the config?
There was a problem hiding this comment.
Ah, good point, this is a bug.
There was a problem hiding this comment.
The current code assumes the fork will be named exactly as specified in the config, and only looks up existing repos by that config name.
There was a problem hiding this comment.
I also need to implement a wait loop here. CreateForkInOrg returns a 202 Accepted immediately because fork creation is asynchronous on GitHub. Currently, we proceed to the next step without verifying the repository exists/is ready. This creates a race condition where subsequent configuration steps (like applying permissions) may fail with a 404 if the fork isn't fully ready yet.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hoxhaeris, petr-muller, Prucek The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
I'm leaning towards keeping it, maybe make it inherit whatever |
|
/reopen dammit why are the buttons so close to each other |
|
@petr-muller: Reopened this PR. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
I'm fine either way as well. The middle ground sounds good -- having Users who want repos managed but not forks can still do I'll update the implementation to follow this approach. |
… fork idempotency bug
k8s-ci-robot
removed
the
lgtm
|
New changes are detected. LGTM label has been removed. |
k8s-ci-robot
added
size/XXL
cc @petr-muller The config key is useful for YAML uniqueness - for example, if we already have a repo named kubernetes and want to fork kubernetes/kubernetes, we can't use duplicate keys in YAML. Current behavior (admittedly weird):
Planned fix: I'm leaning towards supporting non-restrictive metadata changes on forks (e.g., description, has_issues, has_wiki) since those are valid use cases. However, restrictive fields like private should be rejected or warned about, since GitHub doesn't allow making public forks private (although that might not be true for Enterprise, and that is an argument to actually allow all metadata modification, and let the tool fail if the modification is restricted, since trying to anticipate what GitHub will reject is fragile). I'll also need to fix the current issue where metadata is only applied when the config key matches the actual fork name - it should work regardless of what GitHub names the fork. Wanted to get your opinion: does supporting non-restrictive metadata on forks make sense, or should we keep forks minimal (fork_from, default_branch_only, collaborators only)? We can also add support for the name parameter in GitHub's fork API, which would let users enforce a specific fork name via the config key (I am also leaning towards this, more explicit, follows the current behaviour of normal repos).
|
- Add `name` parameter to CreateForkInOrg API to enforce fork names via the config key (GitHub will use this name for the fork) - Reorder configureForks before configureRepos so forkNames mapping is available for metadata updates - Update configureRepos to use actual GitHub name for fork repos via the forkNames mapping (handles renamed forks) - Document fork metadata behavior: fields set in config override inherited values, unset fields keep current values - All metadata modifications are passed through to GitHub; restricted changes (e.g., making public forks private) will result in API errors
Since we always pass the config key as the fork name via GitHub's name parameter, GitHub will either use that exact name or return an error. It will not silently rename the fork. - Remove unreachable check for createdName != repoName - Use repoName directly for waitForFork and forkNames mapping - Remove forkNameOverride test infrastructure (no longer needed) - Update CreateForkInOrg documentation to clarify name behavior
|
/test pull-prow-unit-test |
6 participants
Summary
This PR adds support for managing GitHub repository forks in Peribolos. It allows users to declaratively configure forks alongside standard repositories, handling creation, metadata configuration, and edge cases like naming conflicts.
Key Features
fork_from.nameAPI parameter.Configuration Examples
Basic Fork
Create a fork using the config key as the fork name.
Customized Fork (Hard Fork / Mirror)
Create a fork with a custom name, description, and specific settings (e.g., disabling issues).
Usage
Fork management is enabled automatically when you fix repositories.
Behavior
fork_fromformatMetadata Inheritance
Forks inherit metadata from the upstream repository:
Implementation Details
nameparameter, enforcing the exact fork name. If a repository with this name already exists, the operation fails with an error.configureForkspolls GitHub every 10 seconds until the fork is fully created (up to 5 minutes) before proceeding.RepoMetadatastruct) to align data structures with the GitHub API.API Reference