Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Function "runPromote" is overly complex #1343

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: Function "runPromote" is overly complex #1343

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
181 changes: 111 additions & 70 deletions cmd/kpromo/cmd/pr/pr.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,40 +189,75 @@ func runPromote(opts *promoteOptions) error {
branchname := opts.project + "-" + opts.tags[0] + promotionBranchSuffix

// Get the github org and repo from the fork slug
repo, err := prepareRepository(branchname, opts)
if err != nil {
return err
}

defer func() {
if mustRun(opts, "Clean fork directory?") {
err = repo.Cleanup()
} else {
logrus.Infof("All modified files will be left untouched in %s", repo.Dir())
}
}()

// Handle manifests
modified, err := handleManifests(ctx, repo, opts)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

handleManifests is a bit broad name, what do you think about calling it growManifest instead?

if err != nil {
return err
}

if !modified {
logrus.Info("No changes detected in the promoter images list, exiting without changes")
return nil
}

// Commit and push changes
err = commitAndPushChanges(repo, branchname, opts)
if err != nil {
return err
}

// Create the pull request
if mustRun(opts, "Create pull request?") {
err = createPullRequest(opts, branchname)
if err != nil {
return err
}
}

return nil
}

func prepareRepository(branchname string, opts *promoteOptions) (*git.Repo, error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two nits:

  • given this is about the fork, maybe we should call it prepareFork
  • I have mixed feelings about branchname, I'd prefer branchName, although I see that the variable in rumPromote is called branchname

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additionally, I'd make all these functions that take promoteOptions as an argument, take promoteOptions as a receiver, e.g. func (opts *promoteOptions) prepareRepository (branchname string)

userForkOrg, userForkRepo, err := git.ParseRepoSlug(opts.userFork)
if err != nil {
return fmt.Errorf("parsing user's fork: %w", err)
return nil, fmt.Errorf("parsing user's fork: %w", err)
}
if userForkRepo == "" {
userForkRepo = k8sioRepo
}

// Check Environment
gh := github.New()

// Verify the repository is a fork of k8s.io
if err = github.VerifyFork(
branchname, userForkOrg, userForkRepo, git.DefaultGithubOrg, k8sioRepo,
); err != nil {
return fmt.Errorf("while checking fork of %s/%s: %w ", git.DefaultGithubOrg, k8sioRepo, err)
return nil, fmt.Errorf("while checking fork of %s/%s: %w ", git.DefaultGithubOrg, k8sioRepo, err)
}

// Clone k8s.io
// We might want to set options to pass for the clone, nothing for now
gitCloneOpts := &gogit.CloneOptions{}
repo, err := github.PrepareFork(branchname, git.DefaultGithubOrg, k8sioRepo, userForkOrg, userForkRepo, opts.useSSH, opts.updateRepo, gitCloneOpts)
if err != nil {
return fmt.Errorf("while preparing k/k8s.io fork: %w", err)
return nil, fmt.Errorf("while preparing k/k8s.io fork: %w", err)
}

defer func() {
if mustRun(opts, "Clean fork directory?") {
err = repo.Cleanup()
} else {
logrus.Infof("All modified files will be left untouched in %s", repo.Dir())
}
}()
return repo, nil
}

func handleManifests(ctx context.Context, repo *git.Repo, opts *promoteOptions) (bool, error) {
// Path to the promoter image list
imagesListPath := filepath.Join(
consts.ProdRegistry,
Expand All @@ -232,78 +267,82 @@ func runPromote(opts *promoteOptions) error {
)

// Read the current manifest to check later if new images come up
oldlist := make([]byte, 0)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you change this?

var oldlist []byte
var err error

// Run the promoter manifest grower
if mustRun(opts, "Grow the manifests to add the new tags?") {
if util.Exists(filepath.Join(repo.Dir(), imagesListPath)) {
logrus.Debug("Reading the current image promoter manifest (image list)")
oldlist, err = os.ReadFile(filepath.Join(repo.Dir(), imagesListPath))
if err != nil {
return fmt.Errorf("while reading the current promoter image list: %w", err)
return false, fmt.Errorf("while reading the current promoter image list: %w", err)
}
}

opt := manifest.GrowOptions{}
if err := opt.Populate(
filepath.Join(repo.Dir(), consts.ProdRegistry),
consts.StagingRepoPrefix+opts.project, opts.images, opts.digests, opts.tags); err != nil {
return fmt.Errorf("populating image promoter options for tag %s with image filter %s: %w", opts.tags, opts.images, err)
return false, fmt.Errorf("populating image promoter options for tag %s with image filter %s: %w", opts.tags, opts.images, err)
}

if err := opt.Validate(); err != nil {
return fmt.Errorf("validate promoter options tag %s with image filter %s: %w", opts.tags, opts.images, err)
return false, fmt.Errorf("validate promoter options tag %s with image filter %s: %w", opts.tags, opts.images, err)
}

logrus.Infof("Growing manifests for images matching filter %s and matching tag %s", opts.images, opts.tags)
if err := manifest.Grow(ctx, &opt); err != nil {
return fmt.Errorf("growing manifest with image filter %s and tag %s: %w", opts.images, opts.tags, err)
return false, fmt.Errorf("growing manifest with image filter %s and tag %s: %w", opts.images, opts.tags, err)
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original code is in the mustRun block only up to this point. I believe this function should do the same.


// Re-write the image list without the mock images
rawImageList, err := image.NewManifestListFromFile(filepath.Join(repo.Dir(), imagesListPath))
if err != nil {
return fmt.Errorf("parsing the current manifest: %w", err)
}
// Re-write the image list without the mock images
rawImageList, err := image.NewManifestListFromFile(filepath.Join(repo.Dir(), imagesListPath))
if err != nil {
return false, fmt.Errorf("parsing the current manifest: %w", err)
}

// Create a new imagelist to copy the non-mock images
newImageList := &image.ManifestList{}
// Create a new imagelist to copy the non-mock images
newImageList := &image.ManifestList{}

// Copy all non mock-images:
for _, imageData := range *rawImageList {
if !strings.Contains(imageData.Name, "mock/") {
*newImageList = append(*newImageList, imageData)
// Copy all non mock-images:
for _, imageData := range *rawImageList {
if !strings.Contains(imageData.Name, "mock/") {
*newImageList = append(*newImageList, imageData)
}
}
}

// Write the modified manifest
if err := newImageList.Write(filepath.Join(repo.Dir(), imagesListPath)); err != nil {
return fmt.Errorf("while writing the promoter image list: %w", err)
}
// Write the modified manifest
if err := newImageList.Write(filepath.Join(repo.Dir(), imagesListPath)); err != nil {
return false, fmt.Errorf("while writing the promoter image list: %w", err)
}

// Check if the image list was modified
if len(oldlist) > 0 {
logrus.Debug("Checking if the image list was modified")
// read the newly modified manifest
newlist, err := os.ReadFile(filepath.Join(repo.Dir(), imagesListPath))
if err != nil {
return fmt.Errorf("while reading the modified manifest images list: %w", err)
// Check if the image list was modified
if len(oldlist) > 0 {
logrus.Debug("Checking if the image list was modified")
// read the newly modified manifest
newlist, err := os.ReadFile(filepath.Join(repo.Dir(), imagesListPath))
if err != nil {
return false, fmt.Errorf("while reading the modified manifest images list: %w", err)
}

// If the manifest was not modified, exit now
if bytes.Equal(newlist, oldlist) {
return false, nil
}
}

// If the manifest was not modified, exit now
if bytes.Equal(newlist, oldlist) {
logrus.Info("No changes detected in the promoter images list, exiting without changes")
return nil
// add the modified manifest to staging
logrus.Debugf("Adding %s to staging area", imagesListPath)
if err := repo.Add(imagesListPath); err != nil {
return false, fmt.Errorf("adding image manifest to staging area: %w", err)
}
Comment on lines +335 to 339
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For some reason, this doesn't really feel like part of this function to me. Additionally, if you would put it in the runPronmote function, you could avoid passing repo and instead pass the value of repo.Dir().

}

// add the modified manifest to staging
logrus.Debugf("Adding %s to staging area", imagesListPath)
if err := repo.Add(imagesListPath); err != nil {
return fmt.Errorf("adding image manifest to staging area: %w", err)
}
return true, nil
}

func commitAndPushChanges(repo *git.Repo, branchname string, opts *promoteOptions) error {
commitMessage := "Image promotion for " + opts.project + " " + strings.Join(opts.tags, " / ")
if opts.project == consts.StagingRepoSuffix {
commitMessage = "releng: " + commitMessage
Expand All @@ -316,33 +355,35 @@ func runPromote(opts *promoteOptions) error {
}

// Push to fork
if mustRun(opts, fmt.Sprintf("Push changes to user's fork at %s/%s?", userForkOrg, userForkRepo)) {
logrus.Infof("Pushing manifest changes to %s/%s", userForkOrg, userForkRepo)
if mustRun(opts, fmt.Sprintf("Push changes to user's fork at %s/%s?", opts.userFork, k8sioRepo)) {
logrus.Infof("Pushing manifest changes to %s/%s", opts.userFork, k8sioRepo)
if err := repo.PushToRemote(github.UserForkName, branchname); err != nil {
return fmt.Errorf("pushing %s to %s/%s: %w", github.UserForkName, userForkOrg, userForkRepo, err)
return fmt.Errorf("pushing %s to %s/%s: %w", github.UserForkName, opts.userFork, k8sioRepo, err)
Comment on lines +358 to +361
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks wrong to me, why did you replace userForkOrg with opts.userFork and userForkRepo with k8sioRepo?

}
} else {
// Exit if no push was made

logrus.Infof("Exiting without creating a PR since changes were not pushed to %s/%s", userForkOrg, userForkRepo)
logrus.Infof("Exiting without creating a PR since changes were not pushed to %s/%s", opts.userFork, k8sioRepo)
return nil
Comment on lines +365 to 366
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tool is supposed to exit here, however, it would continue running after this function. You should do something similar as in handleManifests or use os.Exit.

}

// Create the Pull Request
if mustRun(opts, "Create pull request?") {
pr, err := gh.CreatePullRequest(
git.DefaultGithubOrg, k8sioRepo, k8sioDefaultBranch,
fmt.Sprintf("%s:%s", userForkOrg, branchname),
commitMessage, generatePRBody(opts),
)
if err != nil {
return fmt.Errorf("creating the pull request in k/k8s.io: %w", err)
}
logrus.Infof(
"Successfully created PR: %s%s/%s/pull/%d",
github.GitHubURL, git.DefaultGithubOrg, k8sioRepo, pr.GetNumber(),
)
return nil
}

func createPullRequest(opts *promoteOptions, branchname string) error {
gh := github.New()
pr, err := gh.CreatePullRequest(
git.DefaultGithubOrg, k8sioRepo, k8sioDefaultBranch,
fmt.Sprintf("%s:%s", opts.userFork, branchname),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you replace userForkOrg with opts.userFork?

"Image promotion for "+opts.project+" "+strings.Join(opts.tags, " / "),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't match the original behavior, the commit message can be extended in some case, e.g. with the releng: prefix.

generatePRBody(opts),
)
if err != nil {
return fmt.Errorf("creating the pull request in k/k8s.io: %w", err)
}
logrus.Infof(
"Successfully created PR: %s%s/%s/pull/%d",
github.GitHubURL, git.DefaultGithubOrg, k8sioRepo, pr.GetNumber(),
)

// Success!
return nil
Expand Down