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.

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

chore(cli): improve flag help descriptions to show allowed values instead of type names #5067

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion cmd/flux/create_helmrelease_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ func TestCreateHelmRelease(t *testing.T) {
{
name: "unknown source kind",
args: "create helmrelease podinfo --source foobar/podinfo --chart podinfo --export",
assert: assertError(`invalid argument "foobar/podinfo" for "--source" flag: source kind 'foobar' is not supported, must be one of: HelmRepository, GitRepository, Bucket`),
assert: assertError(`invalid argument "foobar/podinfo" for "--source" flag: source kind 'foobar' is not supported, must be one of: HelmRepository|GitRepository|Bucket`),
},
{
name: "unknown chart reference kind",
Expand Down
2 changes: 1 addition & 1 deletion cmd/flux/create_source_chart_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ func TestCreateSourceChart(t *testing.T) {
{
name: "unknown source kind",
args: "create source chart podinfo --source foobar/podinfo --export",
assert: assertError(`invalid argument "foobar/podinfo" for "--source" flag: source kind 'foobar' is not supported, must be one of: HelmRepository, GitRepository, Bucket`),
assert: assertError(`invalid argument "foobar/podinfo" for "--source" flag: source kind 'foobar' is not supported, must be one of: HelmRepository|GitRepository|Bucket`),
},
{
name: "basic chart",
Expand Down
2 changes: 1 addition & 1 deletion cmd/flux/create_source_git_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@ func TestCreateSourceGitExport(t *testing.T) {
{
name: "source with empty provider",
args: "create source git podinfo --namespace=flux-system --url=https://dev.azure.com/foo/bar/_git/podinfo --provider \"\" --branch=test --interval=1m0s --export",
assert: assertError("invalid argument \"\" for \"--provider\" flag: no source Git provider given, please specify the Git provider name"),
assert: assertError("invalid argument \"\" for \"--provider\" flag: no source Git provider given, must be one of: generic|azure"),
},
{
name: "source with no provider",
Expand Down
8 changes: 4 additions & 4 deletions internal/flags/crds.go
Original file line number Diff line number Diff line change
Expand Up @@ -40,21 +40,21 @@ func (a *CRDsPolicy) String() string {
func (a *CRDsPolicy) Set(str string) error {
if strings.TrimSpace(str) == "" {
return fmt.Errorf("no upgrade CRDs policy given, must be one of: %s",
strings.Join(supportedCRDsPolicies, ", "))
a.Type())
}
if !utils.ContainsItemString(supportedCRDsPolicies, str) {
return fmt.Errorf("unsupported upgrade CRDs policy '%s', must be one of: %s",
str, strings.Join(supportedCRDsPolicies, ", "))
str, a.Type())

}
*a = CRDsPolicy(str)
return nil
}

func (a *CRDsPolicy) Type() string {
return "crds"
return strings.Join(supportedCRDsPolicies, "|")
}

func (a *CRDsPolicy) Description() string {
return fmt.Sprintf("upgrade CRDs policy, available options are: (%s)", strings.Join(supportedCRDsPolicies, ", "))
return "upgrade CRDs policy"
}
8 changes: 4 additions & 4 deletions internal/flags/decryption_provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,21 +34,21 @@ func (d *DecryptionProvider) String() string {
func (d *DecryptionProvider) Set(str string) error {
if strings.TrimSpace(str) == "" {
return fmt.Errorf("no decryption provider given, must be one of: %s",
strings.Join(supportedDecryptionProviders, ", "))
d.Type())
}
if !utils.ContainsItemString(supportedDecryptionProviders, str) {
return fmt.Errorf("unsupported decryption provider '%s', must be one of: %s",
str, strings.Join(supportedDecryptionProviders, ", "))
str, d.Type())

}
*d = DecryptionProvider(str)
return nil
}

func (d *DecryptionProvider) Type() string {
return "decryptionProvider"
return strings.Join(supportedDecryptionProviders, "|")
}

func (d *DecryptionProvider) Description() string {
return fmt.Sprintf("decryption provider, available options are: (%s)", strings.Join(supportedDecryptionProviders, ", "))
return "decryption provider"
}
6 changes: 3 additions & 3 deletions internal/flags/ecdsa_curve.go
Original file line number Diff line number Diff line change
Expand Up @@ -45,15 +45,15 @@ func (c *ECDSACurve) Set(str string) error {
*c = ECDSACurve{v}
return nil
}
return fmt.Errorf("unsupported curve '%s', must be one of: %s", str, strings.Join(ecdsaCurves(), ", "))
return fmt.Errorf("unsupported curve '%s', must be one of: %s", str, c.Type())
}

func (c *ECDSACurve) Type() string {
return "ecdsaCurve"
return strings.Join(ecdsaCurves(), "|")
}

func (c *ECDSACurve) Description() string {
return fmt.Sprintf("SSH ECDSA public key curve (%s)", strings.Join(ecdsaCurves(), ", "))
return "SSH ECDSA public key curve"
}

func ecdsaCurves() []string {
Expand Down
16 changes: 13 additions & 3 deletions internal/flags/gitlab_visibility.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package flags

import (
"fmt"
"sort"
"strings"

"github.com/fluxcd/go-git-providers/gitprovider"
Expand Down Expand Up @@ -51,16 +52,25 @@ func (d *GitLabVisibility) Set(str string) error {
}
var visibility = gitprovider.RepositoryVisibility(str)
if ValidateRepositoryVisibility(visibility) != nil {
return fmt.Errorf("unsupported visibility '%s'", str)
return fmt.Errorf("unsupported visibility '%s', must be one of: %s", str, d.Type())
}
*d = GitLabVisibility(visibility)
return nil
}

func (d *GitLabVisibility) Type() string {
return "gitLabVisibility"
return strings.Join(gitLabVisibilities(), "|")
}

func (d *GitLabVisibility) Description() string {
return fmt.Sprintf("specifies the visibility of the repository. Valid values are public, private, internal")
return "specifies the visibility of the repository"
}

func gitLabVisibilities() []string {
visibilities := make([]string, 0, len(supportedGitLabVisibilities))
for visibility := range supportedGitLabVisibilities {
visibilities = append(visibilities, string(visibility))
}
sort.Strings(visibilities)
return visibilities
}
10 changes: 3 additions & 7 deletions internal/flags/helm_chart_source.go
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ func (s *HelmChartSource) Set(str string) error {
cleanSourceKind, ok := utils.ContainsEqualFoldItemString(supportedHelmChartSourceKinds, sourceKind)
if !ok {
return fmt.Errorf("source kind '%s' is not supported, must be one of: %s",
sourceKind, strings.Join(supportedHelmChartSourceKinds, ", "))
sourceKind, s.Type())
}

s.Kind = cleanSourceKind
Expand All @@ -64,13 +64,9 @@ func (s *HelmChartSource) Set(str string) error {
}

func (s *HelmChartSource) Type() string {
return "helmChartSource"
return strings.Join(supportedHelmChartSourceKinds, "|")
}

func (s *HelmChartSource) Description() string {
return fmt.Sprintf(
"source that contains the chart in the format '<kind>/<name>.<namespace>', "+
"where kind must be one of: (%s)",
strings.Join(supportedHelmChartSourceKinds, ", "),
)
return "source that contains the chart in the format '<kind>/<name>.<namespace>'"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

here

Copy link
Contributor

@darkowlzz darkowlzz Nov 15, 2024

Choose a reason for hiding this comment

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

At present (not with this change), this looks like

--source helmChartSource         source that contains the chart in the format '<kind>/<name>.<namespace>', where kind must be one of: (HelmRepository, GitRepository, Bucket)

I think the following may be better

--source <kind>/<name>.<namespace>         source that contains the chart, source kind must be one of: (HelmRepository, GitRepository, Bucket)

I'm afraid that something like HelmRepository|GitRepository|Bucket/<name>.<namespace> could become more confusing as it's a mix of all the allowed values and placeholders. It may be better to just provide the template and list the supported kind values in description.

We can even leave this out of this change as the values are not fixed. But I still think the new format is relatively better than an undefined helmChartSource value.

}
10 changes: 3 additions & 7 deletions internal/flags/kustomization_source.go
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ func (s *KustomizationSource) Set(str string) error {
cleanSourceKind, ok := utils.ContainsEqualFoldItemString(supportedKustomizationSourceKinds, sourceKind)
if !ok {
return fmt.Errorf("source kind '%s' is not supported, must be one of: %s",
sourceKind, strings.Join(supportedKustomizationSourceKinds, ", "))
sourceKind, s.Type())
}

s.Kind = cleanSourceKind
Expand All @@ -71,13 +71,9 @@ func (s *KustomizationSource) Set(str string) error {
}

func (s *KustomizationSource) Type() string {
return "kustomizationSource"
return strings.Join(supportedKustomizationSourceKinds, "|")
}

func (s *KustomizationSource) Description() string {
return fmt.Sprintf(
"source that contains the Kubernetes manifests in the format '[<kind>/]<name>.<namespace>', "+
"where kind must be one of: (%s), if kind is not specified it defaults to GitRepository",
strings.Join(supportedKustomizationSourceKinds, ", "),
)
return "source that contains the Kubernetes manifests in the format '[<kind>/]<name>.<namespace>', if kind is not specified it defaults to GitRepository"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

here

Copy link
Contributor

Choose a reason for hiding this comment

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

Same reasoning as for chart source. Something like

--source [<kind>/]<name>.<namespace>               source that contains the Kubernetes manifests, kind must be one of: (OCIRepository, GitRepository, Bucket), if kind is not specified it defaults to GitRepository

seems like an improvement to me.

}
10 changes: 3 additions & 7 deletions internal/flags/local_helm_chart_source.go
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ func (s *LocalHelmChartSource) Set(str string) error {
cleanSourceKind, ok := utils.ContainsEqualFoldItemString(supportedHelmChartSourceKinds, sourceKind)
if !ok {
return fmt.Errorf("source kind '%s' is not supported, must be one of: %s",
sourceKind, strings.Join(supportedHelmChartSourceKinds, ", "))
sourceKind, s.Type())
}

s.Kind = cleanSourceKind
Expand All @@ -58,13 +58,9 @@ func (s *LocalHelmChartSource) Set(str string) error {
}

func (s *LocalHelmChartSource) Type() string {
return "helmChartSource"
return strings.Join(supportedHelmChartSourceKinds, "|")
}

func (s *LocalHelmChartSource) Description() string {
return fmt.Sprintf(
"source that contains the chart in the format '<kind>/<name>', "+
"where kind must be one of: (%s)",
strings.Join(supportedHelmChartSourceKinds, ", "),
)
return "source that contains the chart in the format '<kind>/<name>'"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

here

Copy link
Contributor

Choose a reason for hiding this comment

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

Same as for chart source mentioned above, just without namespace.

}
8 changes: 4 additions & 4 deletions internal/flags/log_level.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,21 +34,21 @@ func (l *LogLevel) String() string {
func (l *LogLevel) Set(str string) error {
if strings.TrimSpace(str) == "" {
return fmt.Errorf("no log level given, must be one of: %s",
strings.Join(supportedLogLevels, ", "))
l.Type())
}
if !utils.ContainsItemString(supportedLogLevels, str) {
return fmt.Errorf("unsupported log level '%s', must be one of: %s",
str, strings.Join(supportedLogLevels, ", "))
str, l.Type())

}
*l = LogLevel(str)
return nil
}

func (l *LogLevel) Type() string {
return "logLevel"
return strings.Join(supportedLogLevels, "|")
}

func (l *LogLevel) Description() string {
return fmt.Sprintf("log level, available options are: (%s)", strings.Join(supportedLogLevels, ", "))
return "log level"
}
8 changes: 4 additions & 4 deletions internal/flags/public_key_algorithm.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ func (a *PublicKeyAlgorithm) String() string {
func (a *PublicKeyAlgorithm) Set(str string) error {
if strings.TrimSpace(str) == "" {
return fmt.Errorf("no public key algorithm given, must be one of: %s",
strings.Join(supportedPublicKeyAlgorithms, ", "))
a.Type())
}
for _, v := range supportedPublicKeyAlgorithms {
if str == v {
Expand All @@ -41,13 +41,13 @@ func (a *PublicKeyAlgorithm) Set(str string) error {
}
}
return fmt.Errorf("unsupported public key algorithm '%s', must be one of: %s",
str, strings.Join(supportedPublicKeyAlgorithms, ", "))
str, a.Type())
}

func (a *PublicKeyAlgorithm) Type() string {
return "publicKeyAlgorithm"
return strings.Join(supportedPublicKeyAlgorithms, "|")
}

func (a *PublicKeyAlgorithm) Description() string {
return fmt.Sprintf("SSH public key algorithm (%s)", strings.Join(supportedPublicKeyAlgorithms, ", "))
return "SSH public key algorithm"
}
2 changes: 1 addition & 1 deletion internal/flags/rsa_key_bits.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ func (b *RSAKeyBits) Set(str string) error {
}

func (b *RSAKeyBits) Type() string {
return "rsaKeyBits"
return "int"
}

func (b *RSAKeyBits) Description() string {
Expand Down
9 changes: 3 additions & 6 deletions internal/flags/source_bucket_provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -45,19 +45,16 @@ func (p *SourceBucketProvider) Set(str string) error {
}
if !utils.ContainsItemString(supportedSourceBucketProviders, str) {
return fmt.Errorf("source bucket provider '%s' is not supported, must be one of: %v",
str, strings.Join(supportedSourceBucketProviders, ", "))
str, p.Type())
}
*p = SourceBucketProvider(str)
return nil
}

func (p *SourceBucketProvider) Type() string {
return "sourceBucketProvider"
return strings.Join(supportedSourceBucketProviders, "|")
}

func (p *SourceBucketProvider) Description() string {
return fmt.Sprintf(
"the S3 compatible storage provider name, available options are: (%s)",
strings.Join(supportedSourceBucketProviders, ", "),
)
return "the S3 compatible storage provider name"
}
4 changes: 2 additions & 2 deletions internal/flags/source_git_provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,8 @@ func (p *SourceGitProvider) String() string {

func (p *SourceGitProvider) Set(str string) error {
if strings.TrimSpace(str) == "" {
return fmt.Errorf("no source Git provider given, please specify %s",
p.Description())
return fmt.Errorf("no source Git provider given, must be one of: %s",
p.Type())
}
if !utils.ContainsItemString(supportedSourceGitProviders, str) {
return fmt.Errorf("source Git provider '%s' is not supported, must be one of: %v",
Expand Down
9 changes: 3 additions & 6 deletions internal/flags/source_oci_provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -52,21 +52,18 @@ func (p *SourceOCIProvider) Set(str string) error {
}
if !utils.ContainsItemString(supportedSourceOCIProviders, str) {
return fmt.Errorf("source OCI provider '%s' is not supported, must be one of: %v",
str, strings.Join(supportedSourceOCIProviders, ", "))
str, p.Type())
}
*p = SourceOCIProvider(str)
return nil
}

func (p *SourceOCIProvider) Type() string {
return "sourceOCIProvider"
return strings.Join(supportedSourceOCIProviders, "|")
}

func (p *SourceOCIProvider) Description() string {
return fmt.Sprintf(
"the OCI provider name, available options are: (%s)",
strings.Join(supportedSourceOCIProviders, ", "),
)
return "the OCI provider name"
}

func (p *SourceOCIProvider) ToOCIProvider() (oci.Provider, error) {
Expand Down
9 changes: 3 additions & 6 deletions internal/flags/source_oci_verify_provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -40,19 +40,16 @@ func (p *SourceOCIVerifyProvider) Set(str string) error {
}
if !utils.ContainsItemString(supportedSourceOCIVerifyProviders, str) {
return fmt.Errorf("source OCI verify provider '%s' is not supported, must be one of: %v",
str, strings.Join(supportedSourceOCIVerifyProviders, ", "))
str, p.Type())
}
*p = SourceOCIVerifyProvider(str)
return nil
}

func (p *SourceOCIVerifyProvider) Type() string {
return "sourceOCIVerifyProvider"
return strings.Join(supportedSourceOCIVerifyProviders, "|")
}

func (p *SourceOCIVerifyProvider) Description() string {
return fmt.Sprintf(
"the OCI verify provider name to use for signature verification, available options are: (%s)",
strings.Join(supportedSourceOCIVerifyProviders, ", "),
)
return "the OCI verify provider name to use for signature verification"
}