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

Fix regression from #9481 about defaulting prune images arguments #11552

Merged
merged 1 commit into from Oct 26, 2016

Conversation

soltysh
Copy link
Member

@soltysh soltysh commented Oct 25, 2016

Currently oadm prune images has no default arguments, but keepTagRevisions should be 3 and keepYoungerThan should be 60 mins. Upon adding pruneOverSizeLimit that was broken (compare to #9481, here specifically), this commit fixes that problem.
Additionally, I've added logs about what the parameters are set to other prune commands.

@miminar I've mentioned you that on IRC

…ments

Currently oadm prune images has no default arguments, but
keepTagRevisions should be 3 and keepYoungerThan should be 60 mins. Upon
adding pruneOverSizeLimit that was broken, this commit fixes that
problem. Additionally, I've added logs about what the parameters are set
to other prune commands.
@soltysh
Copy link
Member Author

soltysh commented Oct 25, 2016

@mfojtik fyi, since it'll require your approval as well.

Copy link

@miminar miminar left a comment

Choose a reason for hiding this comment

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

Just a minor suggestion regarding logging messages. LGTM otherwise.


glog.V(1).Infof("Creating image pruner with keepYoungerThan=%v, keepTagRevisions=%v, pruneOverSizeLimit=%v",
options.KeepYoungerThan, options.KeepTagRevisions, options.PruneOverSizeLimit)
keepTagRevisions := "<nil>"
Copy link

Choose a reason for hiding this comment

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

"<nil>" reveals an implementation detail. To a regular user, it may look like a magic. Would it be perhaps better not to print unset parameters at all?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's consistent with what we have in other places. I wouldn't worry about this exposing implementation details ;)

@mfojtik
Copy link
Member

mfojtik commented Oct 26, 2016

[test]

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to 81a32a2

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/10663/) (Base Commit: 7cd5ade)

@mfojtik
Copy link
Member

mfojtik commented Oct 26, 2016

[merge]

@mfojtik mfojtik added this to the 1.4.0 milestone Oct 26, 2016
@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to 81a32a2

@openshift-bot
Copy link
Contributor

openshift-bot commented Oct 26, 2016

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/10663/) (Base Commit: 38c9545) (Image: devenv-rhel7_5250)

@openshift-bot openshift-bot merged commit da1b714 into openshift:master Oct 26, 2016
@soltysh soltysh deleted the fix_image_prune branch October 27, 2016 06:09
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.

None yet

4 participants