airship/airshipctl
Code Issues Proposed changes

Fix pull documents for non-master branch

Browse Source 
By specifying any other branch rather than master in airshipctl config,
`airshipctl document pull` command failing with `reference not found`
error.
The resulted target dir, after execution, still pointing to master.
The idea of this change is to use checkout options during the clone
stage, so we will be clonning the specific branch.
Added debug log about cloning default(master) branch in case of
missing checkout options.
Enabled validation for incompatible parameters in checkout options.
Aligned unit tests with code changes.

Closes: #197
Change-Id: I50ac18289a8f02997d5b90c82f1688083cff8bf3
This commit is contained in:
Alexander Noskov 2020-05-01 16:32:11 -05:00
parent f334b45638
commit 753d8a456c
4 changed files with 124 additions and 82 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

33
pkg/config/repo.go
View File

@ -25,6 +25,7 @@ import (
"sigs.k8s.io/yaml"
"opendev.org/airship/airshipctl/pkg/errors"
"opendev.org/airship/airshipctl/pkg/log"
)
// Constants for possible repo authentication types
@ -123,7 +124,7 @@ func (repo *Repository) String() string {
// Validate check possible values for repository and
// returns Error when incorrect value is given
// retruns nill when there are no errors
// returns nil when there are no errors
func (repo *Repository) Validate() error {
if repo.URLString == "" {
return ErrRepoSpecRequiresURL{}
@ -141,6 +142,8 @@ func (repo *Repository) Validate() error {
if err != nil {
return err
}
} else {
log.Debugf("Checkout options not defined, cloning from master")
}
return nil
@ -171,14 +174,15 @@ func (repo *Repository) ToCheckoutOptions(force bool) *git.CheckoutOptions {
co := &git.CheckoutOptions{
Force: force,
}
switch {
case repo.CheckoutOptions == nil:
case repo.CheckoutOptions.Branch != "":
co.Branch = plumbing.NewBranchReferenceName(repo.CheckoutOptions.Branch)
case repo.CheckoutOptions.Tag != "":
co.Branch = plumbing.NewTagReferenceName(repo.CheckoutOptions.Tag)
case repo.CheckoutOptions.CommitHash != "":
co.Hash = plumbing.NewHash(repo.CheckoutOptions.CommitHash)
if repo.CheckoutOptions != nil {
switch {
case repo.CheckoutOptions.Branch != "":
co.Branch = plumbing.NewBranchReferenceName(repo.CheckoutOptions.Branch)
case repo.CheckoutOptions.Tag != "":
co.Branch = plumbing.NewTagReferenceName(repo.CheckoutOptions.Tag)
case repo.CheckoutOptions.CommitHash != "":
co.Hash = plumbing.NewHash(repo.CheckoutOptions.CommitHash)
}
}
return co
}
@ -187,10 +191,19 @@ func (repo *Repository) ToCheckoutOptions(force bool) *git.CheckoutOptions {
// authentication and URL set
// CloneOptions describes how a clone should be performed
func (repo *Repository) ToCloneOptions(auth transport.AuthMethod) *git.CloneOptions {
return &git.CloneOptions{
cl := &git.CloneOptions{
Auth: auth,
URL: repo.URLString,
}
if repo.CheckoutOptions != nil {
switch {
case repo.CheckoutOptions.Branch != "":
cl.ReferenceName = plumbing.NewBranchReferenceName(repo.CheckoutOptions.Branch)
case repo.CheckoutOptions.Tag != "":
cl.ReferenceName = plumbing.NewTagReferenceName(repo.CheckoutOptions.Tag)
}
}
return cl
}
// ToFetchOptions returns an instance of git.FetchOptions for given authentication

16
pkg/config/repo_test.go
View File

@ -31,6 +31,7 @@ const (
toAuthTestName = "ToAuth"
toAuthNilTestName = "ToAuthNil"
ToFetchOptionsTestName = "ToFetchOptions"
ToCloneOptionsTestName = "ToCloneOptions"
toAuthNilError = "toAuthNilError"
URLTestName = "URLTest"
StringTestData = `test-data:
@ -147,6 +148,11 @@ var (
dataMapEntry: []string{"no-auth"},
expectedNil: false,
},
ToCloneOptionsTestName: {
expectError: false,
dataMapEntry: []string{"http-basic-auth", "ssh-key-auth", "no-auth", "empty-checkout"},
expectedNil: false,
},
URLTestName: {
expectError: false,
expectedNil: false,
@ -253,12 +259,18 @@ func TestToCloneOptions(t *testing.T) {
err := yaml.Unmarshal([]byte(StringTestData), data)
require.NoError(t, err)
testCase := TestCaseMap[ToFetchOptionsTestName]
testCase := TestCaseMap[ToCloneOptionsTestName]
for _, name := range testCase.dataMapEntry {
repo := data.TestData[name]
require.NotNil(t, repo)
assert.NotNil(t, repo.ToCloneOptions(nil))
cl := repo.ToCloneOptions(nil)
if testCase.expectedNil {
assert.Nil(t, cl)
} else {
assert.NotNil(t, cl)
assert.NoError(t, cl.Validate())
}
}
}

4
pkg/document/pull/pull.go
View File

@ -44,6 +44,10 @@ func (s *Settings) cloneRepositories() error {
// Clone repositories
for _, extraRepoConfig := range currentManifest.Repositories {
err := extraRepoConfig.Validate()
if err != nil {
return err
}
repository, err := repo.NewRepository(currentManifest.TargetPath, extraRepoConfig)
if err != nil {
return err

153
pkg/document/pull/pull_test.go
View File

@ -21,13 +21,11 @@ import (
"testing"
fixtures "github.com/go-git/go-git-fixtures/v4"
repo2 "opendev.org/airship/airshipctl/pkg/document/repo"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"opendev.org/airship/airshipctl/pkg/config"
"opendev.org/airship/airshipctl/pkg/document/repo"
"opendev.org/airship/airshipctl/pkg/environment"
"opendev.org/airship/airshipctl/pkg/util"
"opendev.org/airship/airshipctl/testutil"
@ -45,82 +43,97 @@ func TestPull(t *testing.T) {
require := require.New(t)
assert := assert.New(t)
t.Run("cloneRepositories", func(t *testing.T) {
dummyPullSettings := getDummyPullSettings()
currentManifest, err := dummyPullSettings.Config.CurrentContextManifest()
require.NoError(err)
fx := fixtures.Basic().One()
dummyGitDir := fx.DotGit().Root()
currentManifest.Repositories = map[string]*config.Repository{currentManifest.PrimaryRepositoryName: {
URLString: dummyGitDir,
CheckoutOptions: &config.RepoCheckout{
tests := []struct {
name string
url string
checkoutOpts *config.RepoCheckout
error error
}{
{
name: "TestCloneRepositoriesValidOpts",
checkoutOpts: &config.RepoCheckout{
Branch: "master",
ForceCheckout: false,
},
Auth: &config.RepoAuth{
Type: "http-basic",
},
error: nil,
},
}
tmpDir, cleanup := testutil.TempDir(t, "airshipctlPullTest-")
defer cleanup(t)
currentManifest.TargetPath = tmpDir
_, err = repo2.NewRepository(".", currentManifest.Repositories[currentManifest.PrimaryRepositoryName])
require.NoError(err)
err = dummyPullSettings.cloneRepositories()
require.NoError(err)
dummyRepoDirName := util.GitDirNameFromURL(dummyGitDir)
assert.FileExists(path.Join(tmpDir, dummyRepoDirName, "go/example.go"))
assert.FileExists(path.Join(tmpDir, dummyRepoDirName, ".git/HEAD"))
contents, err := ioutil.ReadFile(path.Join(tmpDir, dummyRepoDirName, ".git/HEAD"))
require.NoError(err)
assert.Equal("ref: refs/heads/master", strings.TrimRight(string(contents), "\t \n"))
})
t.Run("Pull", func(t *testing.T) {
dummyPullSettings := getDummyPullSettings()
conf := dummyPullSettings.AirshipCTLSettings.Config
fx := fixtures.Basic().One()
mfst := conf.Manifests["dummy_manifest"]
dummyGitDir := fx.DotGit().Root()
mfst.Repositories = map[string]*config.Repository{
mfst.PrimaryRepositoryName: {
URLString: dummyGitDir,
CheckoutOptions: &config.RepoCheckout{
Branch: "master",
ForceCheckout: false,
},
Auth: &config.RepoAuth{
Type: "http-basic",
},
{
name: "TestCloneRepositoriesMissingCheckoutOptions",
error: nil,
},
{
name: "TestCloneRepositoriesNonMasterBranch",
checkoutOpts: &config.RepoCheckout{
Branch: "branch",
ForceCheckout: false,
},
}
dummyPullSettings.Config = conf
error: nil,
},
{
name: "TestCloneRepositoriesInvalidOpts",
checkoutOpts: &config.RepoCheckout{
Branch: "master",
Tag: "someTag",
ForceCheckout: false,
},
error: config.ErrMutuallyExclusiveCheckout{},
},
}
dummyPullSettings := getDummyPullSettings()
currentManifest, err := dummyPullSettings.Config.CurrentContextManifest()
require.NoError(err)
tmpDir, cleanup := testutil.TempDir(t, "airshipctlPullTest-")
defer cleanup(t)
testGitURL := fixtures.Basic().One().URL
dirNameFromURL := util.GitDirNameFromURL(testGitURL)
globalTmpDir, cleanup := testutil.TempDir(t, "airshipctlCloneTest-")
defer cleanup(t)
mfst.TargetPath = tmpDir
for _, tt := range tests {
tmpDir := path.Join(globalTmpDir, tt.name)
expectedErr := tt.error
chkOutOpts := tt.checkoutOpts
t.Run(tt.name, func(t *testing.T) {
currentManifest.Repositories = map[string]*config.Repository{
currentManifest.PrimaryRepositoryName: {
URLString: testGitURL,
CheckoutOptions: chkOutOpts,
Auth: &config.RepoAuth{
Type: "http-basic",
},
},
}
err := dummyPullSettings.Pull()
require.NoError(err)
currentManifest.TargetPath = tmpDir
dummyRepoDirName := util.GitDirNameFromURL(dummyGitDir)
assert.FileExists(path.Join(tmpDir, dummyRepoDirName, "go/example.go"))
assert.FileExists(path.Join(tmpDir, dummyRepoDirName, ".git/HEAD"))
contents, err := ioutil.ReadFile(path.Join(tmpDir, dummyRepoDirName, ".git/HEAD"))
require.NoError(err)
assert.Equal("ref: refs/heads/master", strings.TrimRight(string(contents), "\t \n"))
})
_, err = repo.NewRepository(
".",
currentManifest.Repositories[currentManifest.PrimaryRepositoryName],
)
require.NoError(err)
err = dummyPullSettings.Pull()
if expectedErr != nil {
assert.NotNil(err)
assert.Equal(expectedErr, err)
} else {
require.NoError(err)
assert.FileExists(path.Join(currentManifest.TargetPath, dirNameFromURL, "go/example.go"))
assert.FileExists(path.Join(currentManifest.TargetPath, dirNameFromURL, ".git/HEAD"))
contents, err := ioutil.ReadFile(path.Join(currentManifest.TargetPath, dirNameFromURL, ".git/HEAD"))
require.NoError(err)
if chkOutOpts == nil {
assert.Equal(
"ref: refs/heads/master",
strings.TrimRight(string(contents), "\t \n"),
)
} else {
assert.Equal(
"ref: refs/heads/"+chkOutOpts.Branch,
strings.TrimRight(string(contents), "\t \n"),
)
}
}
})
}
testutil.CleanUpGitFixtures(t)
}