From 0601453b59b854671092329f587bc97397ed41f8 Mon Sep 17 00:00:00 2001 From: James Laverack Date: Sat, 22 Feb 2020 02:30:21 +0000 Subject: [PATCH] Always use a clean checkout for clones during release-notes This avoids a large category of failure modes where an existing on-disk repository is in an unexpected state. We also delete the repo afterwards to avoid multiple runs filling up the disk. This change therefore removes configuration flags for the fork locations. Also included: * Set the upstream when pushing a new branch * Use HTTP transport for git instead of SSH As we are now using the GitHub token for authentication, we need the HTTP URL and not the SSH one. * Remove check for branch existing on remote The existing code does not work for a number of reasons: - It only checks the default remote, not the user's fork. - Any error returned would be treated as a success, not just a "not found". Even without this check there is no risk, as GitHub will simply reject the pushing of a new branch without `--force`, which we do not use. * Use existing branch if it already exists The `-B` flag to `git checkout` will create the branch if it does not exist (existing beaviour) but will also use it if it does exist. --- cmd/krel/cmd/release_notes.go | 67 ++++++++++++++--------------------- docs/krel/release-notes.md | 2 -- pkg/git/git.go | 17 ++++++--- 3 files changed, 39 insertions(+), 47 deletions(-) diff --git a/cmd/krel/cmd/release_notes.go b/cmd/krel/cmd/release_notes.go index c77e6eb4510..a951b456fbc 100644 --- a/cmd/krel/cmd/release_notes.go +++ b/cmd/krel/cmd/release_notes.go @@ -87,17 +87,15 @@ permissions to your fork of k/sig-release and k-sigs/release-notes.`, } type releaseNotesOptions struct { - tag string - draftOrg string - draftRepo string - createDraftPR bool - createWebsitePR bool - outputDir string - sigreleaseForkPath string - kubernetessigsForkPath string - Format string - websiteOrg string - websiteRepo string + tag string + draftOrg string + draftRepo string + createDraftPR bool + createWebsitePR bool + outputDir string + Format string + websiteOrg string + websiteRepo string } type releaseNotesResult struct { @@ -173,20 +171,6 @@ func init() { "The format for notes output (options: markdown, json)", ) - releaseNotesCmd.PersistentFlags().StringVar( - &releaseNotesOpts.kubernetessigsForkPath, - "kubernetes-sigs-fork-path", - filepath.Join(os.TempDir(), "k8s-sigs"), - "fork kubernetes-sigs/release-notes and output a copy of the json release notes to this directory", - ) - - releaseNotesCmd.PersistentFlags().StringVar( - &releaseNotesOpts.sigreleaseForkPath, - "sigrelease-fork-path", - filepath.Join(os.TempDir(), "k8s-sigrelease"), - "fork k/sig-release and output a copy of the release notes draft to this directory", - ) - rootCmd.AddCommand(releaseNotesCmd) } @@ -328,7 +312,7 @@ func validateWebsitePROptions() error { } // createDraftPR pushes the release notes draft to the users fork -func createDraftPR(tag string, result *releaseNotesResult) error { +func createDraftPR(tag string, result *releaseNotesResult) (err error) { s, err := util.TagStringToSemver(tag) if err != nil { return errors.Wrapf(err, "no valid tag: %v", tag) @@ -338,7 +322,7 @@ func createDraftPR(tag string, result *releaseNotesResult) error { // Prepare the fork of k/sig-release sigReleaseRepo, err := prepareFork( - branchname, releaseNotesOpts.sigreleaseForkPath, + branchname, git.DefaultGithubOrg, git.DefaultGithubReleaseRepo, releaseNotesOpts.draftOrg, releaseNotesOpts.draftRepo, ) @@ -346,6 +330,10 @@ func createDraftPR(tag string, result *releaseNotesResult) error { return errors.Wrap(err, "preparing local fork of kubernetes/sig-release") } + defer func() { + err = sigReleaseRepo.Cleanup() + }() + // generate the notes targetdir := filepath.Join(sigReleaseRepo.Dir(), "releases", fmt.Sprintf("release-%d.%d", s.Major, s.Minor)) logrus.Debugf("release notes markdown will be written to %v", targetdir) @@ -375,19 +363,19 @@ func createDraftPR(tag string, result *releaseNotesResult) error { } // prepareFork Prepare a branch a repo -func prepareFork(branchName, repoPath, upstreamOrg, upstreamRepo, myOrg, myRepo string) (repo *git.Repo, err error) { +func prepareFork(branchName, upstreamOrg, upstreamRepo, myOrg, myRepo string) (repo *git.Repo, err error) { // checkout the upstream repository logrus.Infof("cloning/updating repository %s/%s", upstreamOrg, upstreamRepo) - repo, err = git.CloneOrOpenGitHubRepo( - repoPath, upstreamOrg, upstreamRepo, true, + repo, err = git.CleanCloneGitHubRepo( + upstreamOrg, upstreamRepo, false, ) if err != nil { return nil, errors.Wrapf(err, "cloning %s/%s", upstreamOrg, upstreamRepo) } // test if the fork remote is already existing - url := git.GetRepoURL(myOrg, myRepo, true) + url := git.GetRepoURL(myOrg, myRepo, false) if repo.HasRemote(userForkName, url) { logrus.Infof( "Using already existing remote %v (%v) in repository", @@ -401,14 +389,8 @@ func prepareFork(branchName, repoPath, upstreamOrg, upstreamRepo, myOrg, myRepo } } - // verify the branch doesn't already exist on the user's fork - err = repo.HasRemoteBranch(branchName) - if err == nil { - return nil, errors.Errorf("remote repo already has a branch named %s", branchName) - } - // checkout the new branch - err = repo.Checkout("-b", branchName) + err = repo.Checkout("-B", branchName) if err != nil { return nil, errors.Wrapf(err, "creating new branch %s", branchName) } @@ -483,8 +465,8 @@ func processJSONOutput(repoPath string) error { } // createWebsitePR creates the JSON version of the release notes and pushes them to a user fork -func createWebsitePR(tag string, result *releaseNotesResult) error { - _, err := util.TagStringToSemver(tag) +func createWebsitePR(tag string, result *releaseNotesResult) (err error) { + _, err = util.TagStringToSemver(tag) if err != nil { return errors.Wrapf(err, "no valid tag: %v", tag) } @@ -494,12 +476,15 @@ func createWebsitePR(tag string, result *releaseNotesResult) error { // checkout kubernetes-sigs/release-notes k8sSigsRepo, err := prepareFork( - branchname, releaseNotesOpts.kubernetessigsForkPath, defaultKubernetesSigsOrg, + branchname, defaultKubernetesSigsOrg, defaultKubernetesSigsRepo, releaseNotesOpts.websiteOrg, releaseNotesOpts.websiteRepo, ) if err != nil { return errors.Wrap(err, "preparing local fork branch") } + defer func() { + err = k8sSigsRepo.Cleanup() + }() // add a reference to the new json file in assets.ts if err := addReferenceToAssetsFile(k8sSigsRepo.Dir(), jsonNotesFilename); err != nil { diff --git a/docs/krel/release-notes.md b/docs/krel/release-notes.md index 216c6769711..e462572002a 100644 --- a/docs/krel/release-notes.md +++ b/docs/krel/release-notes.md @@ -40,9 +40,7 @@ Before running `krel release-notes` export your GitHub token to \$GITHUB_TOKEN: --draft-repo string the name of the fork of k/sig-release, the Release Notes Draft PR will be created from this repository (default "sig-release") --format string The format for notes output (options: markdown, json) (default "markdown") -h, --help help for release-notes - --kubernetes-sigs-fork-path string fork kubernetes-sigs/release-notes and output a copy of the json release notes to this directory (default "/tmp/k8s-sigs") -o, --output-dir string output a copy of the release notes to this directory (default ".") - --sigrelease-fork-path string fork k/sig-release and output a copy of the release notes draft to this directory (default "/tmp/k8s-sigrelease") -t, --tag string version tag for the notes --website-org string a Github organization owner of the fork of kuberntets-sigs/release-notes where the Website PR will be created --website-repo string the name of the fork of kuberntets-sigs/release-notes, the Release Notes Draft PR will be created from this repository (default "release-notes") diff --git a/pkg/git/git.go b/pkg/git/git.go index 664a8420e2c..b8d7d74fefa 100644 --- a/pkg/git/git.go +++ b/pkg/git/git.go @@ -198,9 +198,18 @@ func CloneOrOpenDefaultGitHubRepoSSH(repoPath string) (*Repo, error) { ) } -// CloneOrOpenGitHubRepo creates a temp directory containing the provided -// GitHub repository via the owner and repo. If useSSH is true, then it will -// clone the repository using the defaultGithubAuthRoot. +// CleanCloneGitHubRepo creates a guaranteed fresh checkout of a given repository. The returned *Repo has a Cleanup() +// method that should be used to delete the repository on-disk afterwards. +func CleanCloneGitHubRepo(owner, repo string, useSSH bool) (*Repo, error) { + repoURL := GetRepoURL(owner, repo, useSSH) + // The use of a blank string for the repo path triggers special behaviour in CloneOrOpenRepo that causes a true + // temporary directory with a random name to be created. + return CloneOrOpenRepo("", repoURL, useSSH) +} + +// CloneOrOpenGitHubRepo works with a repository in the given directory, or creates one if the directory is empty. The +// repo uses the provided GitHub repository via the owner and repo. If useSSH is true, then it will clone the +// repository using the defaultGithubAuthRoot. func CloneOrOpenGitHubRepo(repoPath, owner, repo string, useSSH bool) (*Repo, error) { repoURL := GetRepoURL(owner, repo, useSSH) return CloneOrOpenRepo(repoPath, repoURL, useSSH) @@ -843,7 +852,7 @@ func (r *Repo) AddRemote(name, owner, repo string) error { // PushToRemote push the current branch to a spcified remote, but only if the // repository is not in dry run mode func (r *Repo) PushToRemote(remote, remoteBranch string) error { - args := []string{"push"} + args := []string{"push", "--set-upstream"} if r.dryRun { logrus.Infof("Won't push due to dry run repository") args = append(args, "--dry-run")