Skip to content

cli/command/service: credentialSpecOpt: use strings.Cut#6219

Merged
thaJeztah merged 1 commit intodocker:masterfrom
thaJeztah:cleanup_credentialSpecOpt
Aug 6, 2025
Merged

cli/command/service: credentialSpecOpt: use strings.Cut#6219
thaJeztah merged 1 commit intodocker:masterfrom
thaJeztah:cleanup_credentialSpecOpt

Conversation

@thaJeztah
Copy link
Member

  • Rewrite the function to use strings.Cut instead of checking for, and trimming prefixes for each option.
  • More explicitly set the value, instead of setting an empty value, then propagating the struct.
  • Define a "type" to provide a more enum-like construct.

- Human readable description for the release notes

- A picture of a cute animal (not mandatory but encouraged)

- Rewrite the function to use strings.Cut instead of checking for,
  and trimming prefixes for each option.
- More explicitly set the value, instead of setting an empty value,
  then propagating the struct.
- Define a "type" to provide a more enum-like construct.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah thaJeztah added area/swarm kind/refactor PR's that refactor, or clean-up code labels Jul 29, 2025
Comment on lines +347 to 348
// TODO(thaJeztah): should c.source always be set, even if we may error further down?
c.source = value
Copy link
Member Author

Choose a reason for hiding this comment

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

This was existing behavior, but felt a bit "odd" to set the value regardless if it was valid.

Copy link
Member Author

@thaJeztah thaJeztah Aug 5, 2025

Choose a reason for hiding this comment

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

Looking at go stdlib flags; the Set validates before setting the value (c.source is what's returned by c.String() for us); https://github.com/golang/go/blob/7a1679d7ae32dd8a01bd355413ee77ba517f5f43/src/flag/flag.go#L221-L233

func newUint64Value(val uint64, p *uint64) *uint64Value {
	*p = val
	return (*uint64Value)(p)
}

func (i *uint64Value) Set(s string) error {
	v, err := strconv.ParseUint(s, 0, 64)
	if err != nil {
		err = numError(err)
	}
	*i = uint64Value(v)
	return err
}

spf13/pflags also sets the value regardless (although the function used may return a default value);

func newUint64Value(val uint64, p *uint64) *uint64Value {
*p = val
return (*uint64Value)(p)
}
func (i *uint64Value) Set(s string) error {
v, err := strconv.ParseUint(s, 0, 64)
*i = uint64Value(v)
return err
}
func (i *uint64Value) Type() string {
return "uint64"
}
func (i *uint64Value) String() string { return strconv.FormatUint(uint64(*i), 10) }

@codecov-commenter
Copy link

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

Comment on lines +385 to 386
c.value = &swarm.CredentialSpec{}
return errors.New(`invalid credential spec: value must be prefixed with "config://", "file://", or "registry://"`)
Copy link
Member Author

Choose a reason for hiding this comment

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

This was also the previous behavior (where the value would be set to an empty struct regardless), but not sure if the value is used if an error is returned, so perhaps either skipping, or setting it to nil could work?

@thaJeztah thaJeztah merged commit 25f9587 into docker:master Aug 6, 2025
111 of 112 checks passed
@thaJeztah thaJeztah deleted the cleanup_credentialSpecOpt branch August 6, 2025 08:09
@thaJeztah thaJeztah added this to the 29.0.0 milestone Aug 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/swarm kind/refactor PR's that refactor, or clean-up code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants