Conversation
|
From the original issue I am not sure if this feature is still needed. |
20d5717 to
de4a303
Compare
| default=7, | ||
| option='--image-cache-age', | ||
| metavar='DAYS', | ||
| help="Maximum age of cached image URLs in days, 7 by default.", |
There was a problem hiding this comment.
Click & co. will take care of displaying the default value, please, don't include it in the help text. It would be slightly different if the behavior was more complex, but simple values like this and image-cache do not need to be mentioned.
There was a problem hiding this comment.
I don't think we need to limit ourselves tyo just days, as we have Pint available, which cn convert between units easily. See e.g. log_size_limit in tmt.steps.report.reportportal, it's using Size instead of int, and employes special conversions to convert from the input - 1 kB, 10 GB, ... - to a value in bytes. I think we should support something similar here, and allow input like 1d, 3600s, or 72h. It would require the change of this field type, and addition of custom callbacks for conversion.
There was a problem hiding this comment.
Click & co. will take care of displaying the default value, please, don't include it in the help text. It would be slightly different if the behavior was more complex, but simple values like this and
image-cachedo not need to be mentioned.
Addressed in 88109c0
There was a problem hiding this comment.
I don't think we need to limit ourselves tyo just days, as we have Pint available, which cn convert between units easily. See e.g.
log_size_limitintmt.steps.report.reportportal, it's usingSizeinstead ofint, and employes special conversions to convert from the input -1 kB,10 GB, ... - to a value in bytes. I think we should support something similar here, and allow input like1d,3600s, or72h. It would require the change of this field type, and addition of custom callbacks for conversion.
Addressed in df3892a
There was a problem hiding this comment.
Well also add show_default=True 🙂
| help="List locally available images.", | ||
| ) | ||
|
|
||
| image_cache: bool = field( |
There was a problem hiding this comment.
At first I would expect this to be a path to the "image cache". How about use-image-cache or similar?
| def prepare_image() -> None: | ||
| self._image = testcloud.image.Image(self.image_url) | ||
| self.verbose('qcow', self._image.name, 'green') | ||
|
|
|
@HabibRh26, please update the pull request title and description to match the actual changes. |
de4a303 to
893f09e
Compare
HabibRh26
changed the title
updated |
| are treated as days for backward compatibility. | ||
| """ | ||
|
|
||
| from pint import Quantity |
There was a problem hiding this comment.
We use global imports unless strictly needed by circular dependencies
There was a problem hiding this comment.
followed the existing pattern from normalize_data_amount
There was a problem hiding this comment.
Hmm, that may have been an artifact from when this was an optional dependency. It is a mandatory dependency now, so can move it (and the others) to global import
| ) | ||
|
|
||
| image_cache_age: 'Size' = field( | ||
| default=tmt.hardware.UNITS('7 day'), |
There was a problem hiding this comment.
I think you can change this to string and normalize would still be applied. Then show_default and show it in a nicer format
There was a problem hiding this comment.
Correct, '7 days' should work.
There was a problem hiding this comment.
Seems like our assumption is incorrect here 🤔 something broke with this change
therazix
added
plugin | testcloud
893f09e to
379f702
Compare
379f702 to
271e592
Compare
6 participants
Pull Request Checklist