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

Allow 'until' filters to accept formats as described in docs #40254

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bunker-inspector
Copy link

- What I did
Added support for more until parameters for system prune --filter to match the docs: https://docs.docker.com/engine/reference/commandline/system_prune/
Fixes #40253

- How I did it
This attempts to interpret the user input as each supported format in order, accepting the first that doesn't return an error.

- How to verify it
Run docker system prune --force --all --filter "until=<ts-or-date-string>" using the listed supported formats from the docs

- Description for the changelog
system prune --filter "until" parameter allows all formats listed on in the docs

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

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

Thanks for contributing! Looking for how this is handled for the other "prune" endpoints, I noticed that there's already an implementation for this, so it may be better to use that one (see my comment inline); could you update your PR to use that?

(When updating, please amend your current commit so that there's a single commit in this PR, but feel to message me on the community slack if you need more instructions)

@@ -587,7 +587,39 @@ func toBuildkitPruneInfo(opts types.BuildCachePruneOptions) (client.PruneInfo, e
// nothing to do
case 1:
var err error
until, err = time.ParseDuration(untilValues[0])
Copy link
Member

Choose a reason for hiding this comment

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

Instead of implementing the parsing here, I think it would be better to use the same implementation as is used by the other endpoints (see the original implementation in #29226); https://github.com/moby/moby/blob/48cfe3f0872647bf0f52e6ba65022e1859e808e8/api/types/time/timestamp.go

(TBH, in hindsight, we should've done the parsing itself in the client, and always send a fixed format timestamp over the API, but I guess that's too late to change)

Copy link
Author

Choose a reason for hiding this comment

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

Good catch! I've updated it to use the method you've suggested.

@bunker-inspector bunker-inspector force-pushed the 40235-more-prune-until-formats branch 5 times, most recently from a8cd477 to 812dbb5 Compare December 1, 2019 22:25
if len(tsComponents) == 2 {
nsecs, _ = strconv.Atoi(tsComponents[1])
} else {
nsecs = 0
Copy link
Member

Choose a reason for hiding this comment

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

you can leave out the "else" here, because var nsecs int will already default to 0

But looking tat this part, isn't this doing the same as apitime. ParseTimestamps() ?

func ParseTimestamps(value string, def int64) (int64, int64, error) {
if value == "" {
return def, 0, nil
}
return parseTimestamp(value)
}
func parseTimestamp(value string) (int64, int64, error) {
sa := strings.SplitN(value, ".", 2)
s, err := strconv.ParseInt(sa[0], 10, 64)
if err != nil {
return s, 0, err
}
if len(sa) != 2 {
return s, 0, nil
}
n, err := strconv.ParseInt(sa[1], 10, 64)
if err != nil {
return s, n, err
}
// should already be in nanoseconds but just in case convert n to nanoseconds
n = int64(float64(n) * math.Pow(float64(10), float64(9-len(sa[1]))))
return s, n, nil
}

if err != nil {
return client.PruneInfo{}, errors.Wrapf(err, "%q filter expects a duration (e.g., '24h')", filterKey)
} else {
Copy link
Member

Choose a reason for hiding this comment

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

The else looks redundant here, because we wouldn't reach this point if there was an error.

Signed-off-by: Ted Kassen <tedgkassen@gmail.com>
@git4lad
Copy link

git4lad commented Nov 18, 2021

is there any plan to merge this change ? if not are we planning to update the document to say we don't support date format ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

timestamps don't work in docker system prune's 'until' filter
3 participants