airship/airshipctl
Code Issues Proposed changes

Fix various code style issues

Browse Source 
Fixes possible name collisions between variable names and package names

Remove redundant import naming

Use nil slices for slice declarations instead of empty slices

Use make for slices of fixed lengths

Remove redundant parentheses

Replace deprecated `SetOutput` method with `SetOut`

Fix swapped actual/expected arguments on assertEqualGolden

Change-Id: Ia39ef44372c3e44948e5440575125bdb470898df
This commit is contained in:
Ian H. Pittwood 2020-02-05 16:26:19 -06:00 committed by Ian H Pittwood
parent cac75584d0
commit c7c1011a5c
24 changed files with 71 additions and 71 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
cmd/bootstrap/bootstrap_remotedirect.go
View File

@ -5,7 +5,7 @@ import (
"opendev.org/airship/airshipctl/pkg/environment"
alog "opendev.org/airship/airshipctl/pkg/log"
remote "opendev.org/airship/airshipctl/pkg/remote"
"opendev.org/airship/airshipctl/pkg/remote"
)
// RemoteDirect settings for remotedirect command

2
cmd/completion/completion.go
View File

@ -27,7 +27,7 @@ var (
)
func NewCompletionCommand() *cobra.Command {
shells := []string{}
shells := make([]string, 0, len(completionShells))
for s := range completionShells {
shells = append(shells, s)
}

6
cmd/config/config.go
View File

@ -11,9 +11,9 @@ func NewConfigCommand(rootSettings *environment.AirshipCTLSettings) *cobra.Comma
configRootCmd := &cobra.Command{
Use: "config",
DisableFlagsInUseLine: true,
Short: ("Modify airshipctl config files"),
Long: (`Modify airshipctl config files using subcommands
like "airshipctl config set-context --current-context my-context" `),
Short: "Modify airshipctl config files",
Long: `Modify airshipctl config files using subcommands
like "airshipctl config set-context --current-context my-context" `,
}
configRootCmd.AddCommand(NewCmdConfigSetCluster(rootSettings))
configRootCmd.AddCommand(NewCmdConfigGetCluster(rootSettings))

6
cmd/config/get_authinfo.go
View File

@ -27,13 +27,13 @@ import (
)
var (
getAuthInfoLong = (`Display a specific user information, or all defined users if no name is provided`)
getAuthInfoLong = `Display a specific user information, or all defined users if no name is provided`
getAuthInfoExample = (`# List all the users airshipctl knows about
getAuthInfoExample = `# List all the users airshipctl knows about
airshipctl config get-credential
# Display a specific user information
airshipctl config get-credential e2e`)
airshipctl config get-credential e2e`
)
// An AuthInfo refers to a particular user for a cluster

2
cmd/config/get_cluster.go
View File

@ -27,7 +27,7 @@ import (
)
var (
getClusterLong = (`Display a specific cluster or all defined clusters if no name is provided`)
getClusterLong = "Display a specific cluster or all defined clusters if no name is provided"
getClusterExample = fmt.Sprintf(`
# List all the clusters airshipctl knows about

2
cmd/config/get_context.go
View File

@ -27,7 +27,7 @@ import (
)
var (
getContextLong = (`Display a specific context, the current-context or all defined contexts if no name is provided`)
getContextLong = "Display a specific context, the current-context or all defined contexts if no name is provided"
getContextExample = fmt.Sprintf(`# List all the contexts airshipctl knows about
airshipctl config get-context

4
cmd/config/get_context_test.go
View File

@ -72,8 +72,8 @@ func TestGetContextCmd(t *testing.T) {
Name: "missing",
CmdLine: fmt.Sprintf("%s", missingContext),
Cmd: cmd.NewCmdConfigGetContext(settings),
Error: fmt.Errorf("Context %s information was not "+
"found in the configuration.", missingContext),
Error: fmt.Errorf(`Context %s information was not
found in the configuration.`, missingContext),
},
{
Name: "get-current-context",

2
cmd/config/init.go
View File

@ -24,7 +24,7 @@ import (
)
var (
configInitLong = (`Generate initial configuration files for airshipctl`)
configInitLong = "Generate initial configuration files for airshipctl"
)
// NewCmdConfigInit returns a Command instance for 'config init' sub command

2
cmd/config/set_authinfo_test.go
View File

@ -125,7 +125,7 @@ func (test setAuthInfoTest) run(t *testing.T) {
buf := bytes.NewBuffer([]byte{})
cmd := NewCmdConfigSetAuthInfo(settings)
cmd.SetOutput(buf)
cmd.SetOut(buf)
cmd.SetArgs(test.args)
err := cmd.Flags().Parse(test.flags)
require.NoErrorf(t, err, "unexpected error flags args to command: %v, flags: %v", err, test.flags)

4
cmd/config/set_cluster.go
View File

@ -28,9 +28,9 @@ import (
)
var (
setClusterLong = (`
setClusterLong = `
Sets a cluster entry in arshipctl config.
Specifying a name that already exists will merge new fields on top of existing values for those fields.`)
Specifying a name that already exists will merge new fields on top of existing values for those fields.`
setClusterExample = fmt.Sprintf(`
# Set only the server field on the e2e cluster entry without touching other values.

2
cmd/config/set_cluster_test.go
View File

@ -194,7 +194,7 @@ func (test setClusterTest) run(t *testing.T) {
buf := bytes.NewBuffer([]byte{})
cmd := NewCmdConfigSetCluster(settings)
cmd.SetOutput(buf)
cmd.SetOut(buf)
cmd.SetArgs(test.args)
err := cmd.Flags().Parse(test.flags)
require.NoErrorf(t, err, "unexpected error flags args to command: %v, flags: %v", err, test.flags)

4
cmd/config/set_context.go
View File

@ -27,9 +27,9 @@ import (
)
var (
setContextLong = (`
setContextLong = `
Sets a context entry in arshipctl config.
Specifying a name that already exists will merge new fields on top of existing values for those fields.`)
Specifying a name that already exists will merge new fields on top of existing values for those fields.`
setContextExample = fmt.Sprintf(`
# Create a completely new e2e context entry

2
cmd/config/set_context_test.go
View File

@ -154,7 +154,7 @@ func (test setContextTest) run(t *testing.T) {
buf := bytes.NewBuffer([]byte{})
cmd := NewCmdConfigSetContext(settings)
cmd.SetOutput(buf)
cmd.SetOut(buf)
cmd.SetArgs(test.args)
err := cmd.Flags().Parse(test.flags)
require.NoErrorf(t, err, "unexpected error flags args to command: %v, flags: %v", err, test.flags)

2
cmd/root.go
View File

@ -43,7 +43,7 @@ func NewRootCmd(out io.Writer) (*cobra.Command, *environment.AirshipCTLSettings,
settings.InitConfig()
},
}
rootCmd.SetOutput(out)
rootCmd.SetOut(out)
rootCmd.AddCommand(NewVersionCommand())
settings.InitFlags(rootCmd)

62
pkg/config/config.go
View File

@ -367,12 +367,12 @@ func (c *Config) PersistConfig() error {
}
func (c *Config) String() string {
yaml, err := c.ToYaml()
yamlData, err := c.ToYaml()
// This is hiding the error perhaps
if err != nil {
return ""
}
return string(yaml)
return string(yamlData)
}
func (c *Config) ToYaml() ([]byte, error) {
@ -398,18 +398,18 @@ func (c *Config) KubeConfig() *kubeconfig.Config {
}
func (c *Config) ClusterNames() []string {
names := []string{}
for k := range c.Clusters {
names = append(names, k)
names := make([]string, 0, len(c.Clusters))
for c := range c.Clusters {
names = append(names, c)
}
sort.Strings(names)
return names
}
func (c *Config) ContextNames() []string {
names := []string{}
for k := range c.Contexts {
names = append(names, k)
names := make([]string, 0, len(c.Contexts))
for c := range c.Contexts {
names = append(names, c)
}
sort.Strings(names)
return names
@ -498,7 +498,7 @@ func (c *Config) ModifyCluster(cluster *Cluster, theCluster *ClusterOptions) (*C
}
func (c *Config) GetClusters() ([]*Cluster, error) {
clusters := []*Cluster{}
clusters := make([]*Cluster, 0, len(c.ClusterNames()))
for _, cName := range c.ClusterNames() {
for _, ctName := range AllClusterTypes {
cluster, err := c.GetCluster(cName, ctName)
@ -524,7 +524,7 @@ func (c *Config) GetContext(cName string) (*Context, error) {
}
func (c *Config) GetContexts() ([]*Context, error) {
contexts := []*Context{}
contexts := make([]*Context, 0, len(c.ContextNames()))
// Given that we change the testing metholdogy
// The ordered names are no longer required
for _, cName := range c.ContextNames() {
@ -630,7 +630,7 @@ func (c *Config) GetAuthInfo(aiName string) (*AuthInfo, error) {
}
func (c *Config) GetAuthInfos() ([]*AuthInfo, error) {
authinfos := []*AuthInfo{}
authinfos := make([]*AuthInfo, 0, len(c.AuthInfos))
for cName := range c.AuthInfos {
authinfo, err := c.GetAuthInfo(cName)
if err == nil {
@ -825,11 +825,11 @@ func (m *Manifest) Equal(n *Manifest) bool {
}
func (m *Manifest) String() string {
yaml, err := yaml.Marshal(&m)
yamlData, err := yaml.Marshal(&m)
if err != nil {
return ""
}
return string(yaml)
return string(yamlData)
}
// Repository functions
@ -839,21 +839,21 @@ func (r *Repository) Equal(s *Repository) bool {
}
var urlMatches bool
if r.Url != nil && s.Url != nil {
urlMatches = (r.Url.String() == s.Url.String())
urlMatches = r.Url.String() == s.Url.String()
} else {
// this catches cases where one or both are nil
urlMatches = (r.Url == s.Url)
urlMatches = r.Url == s.Url
}
return urlMatches &&
r.Username == s.Username &&
r.TargetPath == s.TargetPath
}
func (r *Repository) String() string {
yaml, err := yaml.Marshal(&r)
yamlData, err := yaml.Marshal(&r)
if err != nil {
return ""
}
return string(yaml)
return string(yamlData)
}
// Modules functions
@ -864,11 +864,11 @@ func (m *Modules) Equal(n *Modules) bool {
return reflect.DeepEqual(m.BootstrapInfo, n.BootstrapInfo)
}
func (m *Modules) String() string {
yaml, err := yaml.Marshal(&m)
yamlData, err := yaml.Marshal(&m)
if err != nil {
return ""
}
return string(yaml)
return string(yamlData)
}
// Bootstrap functions
@ -882,11 +882,11 @@ func (b *Bootstrap) Equal(c *Bootstrap) bool {
}
func (b *Bootstrap) String() string {
yaml, err := yaml.Marshal(&b)
yamlData, err := yaml.Marshal(&b)
if err != nil {
return ""
}
return string(yaml)
return string(yamlData)
}
// Container functions
@ -900,11 +900,11 @@ func (c *Container) Equal(d *Container) bool {
}
func (c *Container) String() string {
yaml, err := yaml.Marshal(&c)
yamlData, err := yaml.Marshal(&c)
if err != nil {
return ""
}
return string(yaml)
return string(yamlData)
}
// Builder functions
@ -918,11 +918,11 @@ func (b *Builder) Equal(c *Builder) bool {
}
func (b *Builder) String() string {
yaml, err := yaml.Marshal(&b)
yamlData, err := yaml.Marshal(&b)
if err != nil {
return ""
}
return string(yaml)
return string(yamlData)
}
// ClusterComplexName functions
@ -980,26 +980,26 @@ HAS SOMETHING LIKE THIS
*/
func KClusterString(kCluster *kubeconfig.Cluster) string {
yaml, err := yaml.Marshal(&kCluster)
yamlData, err := yaml.Marshal(&kCluster)
if err != nil {
return ""
}
return string(yaml)
return string(yamlData)
}
func KContextString(kContext *kubeconfig.Context) string {
yaml, err := yaml.Marshal(&kContext)
yamlData, err := yaml.Marshal(&kContext)
if err != nil {
return ""
}
return string(yaml)
return string(yamlData)
}
func KAuthInfoString(kAuthInfo *kubeconfig.AuthInfo) string {
yaml, err := yaml.Marshal(&kAuthInfo)
yamlData, err := yaml.Marshal(&kAuthInfo)
if err != nil {
return ""
}
return string(yaml)
return string(yamlData)
}

4
pkg/config/test_utils.go
View File

@ -95,9 +95,9 @@ func DummyManifest() *Manifest {
func DummyRepository() *Repository {
// TODO(howell): handle this error
//nolint: errcheck
url, _ := url.Parse("http://dummy.url.com")
parsedUrl, _ := url.Parse("http://dummy.url.com")
return &Repository{
Url: url,
Url: parsedUrl,
Username: "dummy_user",
TargetPath: "dummy_targetpath",
}

14
pkg/document/bundle.go
View File

@ -155,14 +155,14 @@ func (b *BundleFactory) GetFileSystem() fs.FileSystem {
// GetAllDocuments returns all documents in this bundle
func (b *BundleFactory) GetAllDocuments() ([]Document, error) {
docSet := []Document{}
for _, res := range b.ResMap.Resources() {
docSet := make([]Document, len(b.ResMap.Resources()))
for i, res := range b.ResMap.Resources() {
// Construct Bundle document for each resource returned
doc, err := NewDocument(res)
if err != nil {
return docSet, err
}
docSet = append(docSet, doc)
docSet[i] = doc
}
return docSet, nil
}
@ -170,7 +170,7 @@ func (b *BundleFactory) GetAllDocuments() ([]Document, error) {
// GetByName finds a document by name, error if more than one document found
// or if no documents found
func (b *BundleFactory) GetByName(name string) (Document, error) {
resSet := []*resource.Resource{}
resSet := make([]*resource.Resource, 0, len(b.ResMap.Resources()))
for _, res := range b.ResMap.Resources() {
if res.GetName() == name {
resSet = append(resSet, res)
@ -198,14 +198,14 @@ func (b *BundleFactory) Select(selector types.Selector) ([]Document, error) {
}
// Construct Bundle document for each resource returned
docSet := []Document{}
for _, res := range resources {
docSet := make([]Document, len(resources))
for i, res := range resources {
var doc Document
doc, err = NewDocument(res)
if err != nil {
return docSet, err
}
docSet = append(docSet, doc)
docSet[i] = doc
}
return docSet, err
}

2
pkg/document/document_test.go
View File

@ -30,7 +30,7 @@ func TestDocument(t *testing.T) {
docs, err := bundle.GetAllDocuments()
require.NoError(err, "Unexpected error trying to GetAllDocuments")
nameList := []string{}
nameList := make([]string, 0, len(docs))
for _, doc := range docs {
nameList = append(nameList, doc.GetName())

4
pkg/remote/redfish/redfish.go
View File

@ -105,14 +105,14 @@ func NewRedfishRemoteDirectClient(ctx context.Context,
var api redfishApi.RedfishAPI = redfishClient.NewAPIClient(cfg).DefaultApi
url, err := url.Parse(remoteURL)
parsedUrl, err := url.Parse(remoteURL)
if err != nil {
return RedfishRemoteDirect{}, NewRedfishConfigErrorf("Invalid URL format: %v", err)
}
client := RedfishRemoteDirect{
Context: ctx,
RemoteURL: *url,
RemoteURL: *parsedUrl,
EphemeralNodeId: ephNodeID,
IsoPath: isoPath,
Api: api,

4
pkg/remote/redfish/utils.go
View File

@ -25,8 +25,8 @@ func GetResourceIDFromURL(refURL string) string {
log.Fatal(err)
}
url := strings.TrimSuffix(u.Path, "/")
elems := strings.Split(url, "/")
trimmedUrl := strings.TrimSuffix(u.Path, "/")
elems := strings.Split(trimmedUrl, "/")
id := elems[len(elems)-1]
return id

2
pkg/remote/redfish/utils_test.go
View File

@ -85,7 +85,7 @@ func TestRedfishUtilIsIdInList(t *testing.T) {
{OdataId: "/path/to/id/3"},
{OdataId: "/path/to/id/4"},
}
emptyList := []redfishClient.IdRef{}
var emptyList []redfishClient.IdRef
res := IsIDInList(idList, "1")
assert.True(t, res)

2
pkg/remote/remote_direct.go
View File

@ -4,7 +4,7 @@ import (
"context"
alog "opendev.org/airship/airshipctl/pkg/log"
redfish "opendev.org/airship/airshipctl/pkg/remote/redfish"
"opendev.org/airship/airshipctl/pkg/remote/redfish"
)
const (

2
pkg/remote/remote_direct_test.go
View File

@ -5,7 +5,7 @@ import (
"github.com/stretchr/testify/assert"
redfish "opendev.org/airship/airshipctl/pkg/remote/redfish"
"opendev.org/airship/airshipctl/pkg/remote/redfish"
)
func TestUnknownRemoteType(t *testing.T) {

4
testutil/utilities.go
View File

@ -48,7 +48,7 @@ func RunTest(t *testing.T, test *CmdTest) {
cmd := test.Cmd
actual := &bytes.Buffer{}
cmd.SetOutput(actual)
cmd.SetOut(actual)
args := strings.Fields(test.CmdLine)
cmd.SetArgs(args)
@ -95,7 +95,7 @@ func assertEqualGolden(t *testing.T, test *CmdTest, actual []byte) {
goldenFilePath := filepath.Join(goldenDir, test.Name+goldenFileSuffix)
golden, err := ioutil.ReadFile(goldenFilePath)
require.NoErrorf(t, err, "Failed while reading golden file at %s", goldenFilePath)
assert.Equal(t, string(actual), string(golden))
assert.Equal(t, string(golden), string(actual))
}
func checkError(t *testing.T, actual, expected error) {