RemoveOldRepodataStep for yum publisher #1056
Conversation
|
@midnightercz, thanks for your PR! By analyzing the history of the files in this pull request, we identified @asmacdo, @mhrivnak and @goosemania to be potential reviewers. |
|
Can one of the admins verify this patch? |
1 similar comment
|
Can one of the admins verify this patch? |
|
ok test |
There was a problem hiding this comment.
Thanks @midnightercz. I kicked off the unit tests, so we'll see how they go. Since this is new functionality can this be documented with:
- documentation in the yum publisher stating this behavior
- a release note that also links to the yum publisher docs
Also the 14 days is more than a default, it's hard coded. Should this be user configurable? It seems like it should be. What do you think?
|
Can one of the admins verify this patch? |
|
We chose 14 days as maximal threshold. I'm not sure where we originally get this value. Threshold is configurable via remove_old_repodata_threshold attribute. I will work on docs. I also notice a left some debug prints in the code, so I will push new update anyway. |
docs/tech-reference/yum-plugins.rst
Outdated
| If ``remove_old_repodata`` is true, this attribute specify maximal age of | ||
| repodata in seconds that are not removed during publish. Default is 2 weeks. | ||
|
|
||
|
|
docs/tech-reference/yum-plugins.rst
Outdated
|
|
||
| ``remove_old_repodata_threshold`` | ||
| If ``remove_old_repodata`` is true, this attribute specify maximal age of | ||
| repodata in seconds that are not removed during publish. Default is 2 weeks. |
There was a problem hiding this comment.
s/Default is/Hard coded to/. Since it can't be changed by the user we need to claim it's hard coded not a default that is configurable.
|
Can you add your name to the AUTHORS file in the root of the repo? |
|
A release note is needed for pulp_rpm also. Can you add a description of the feature to the 2.14 release notes? https://github.com/pulp/pulp_rpm/blob/master/docs/user-guide/release-notes/2.14.x.rst |
There was a problem hiding this comment.
I left four easy to handle comments. Can those be handled? Then I think this will be ready for merging. Thanks @midnightercz !
|
There are also flake8 environment errors. The test runners run tests and check for flake8. Those need to all show green before we can merge. |
| matched = exp.match(fname) | ||
| if matched: | ||
| delta = datetime.datetime.today() -\ | ||
| datetime.datetime.fromtimestamp(os.path.getmtime(f)) |
There was a problem hiding this comment.
Does this depend on https://pulp.plan.io/issues/2783 being implemented first? Without that, the mtime will always be equal to the time of the CopyDirectoryStep and so I expect this will never remove anything.
There was a problem hiding this comment.
I'm not sure so I'm defer to @midnightercz
| @@ -10,3 +10,6 @@ New Features | |||
|
|
|||
| * New parameter for rsync distributor ``rsync_extra_args`` allows user to | |||
| specify custom arguments for rsync call during publish operations. | |||
|
|
|||
| * New publish step RemoveOldRepodataStep removes expired repodata at the end | |||
There was a problem hiding this comment.
s/expired repodata/repodata older than 14 days/
There was a problem hiding this comment.
Please add a sentence that says Enable this step with these new distributor options. and have the 'these new distributor options' part link to your new settings in the docs please.
docs/tech-reference/yum-plugins.rst
Outdated
|
|
||
| ``remove_old_repodata`` | ||
| Boolean flag to indicate whether or not repodata passed expiration are removed | ||
| during publish. Files that present in repomd.xml are preserved no matter of |
There was a problem hiding this comment.
Can you add two sentences in the middle here: If not present it defaults to False. The default expiration is 14 days.
There was a problem hiding this comment.
If not present it defaults to True
docs/tech-reference/yum-plugins.rst
Outdated
|
|
||
| ``remove_old_repodata_threshold`` | ||
| If ``remove_old_repodata`` is true, this attribute specify maximal age of | ||
| repodata in seconds that are not removed during publish. Hard coded to 2 weeks. |
There was a problem hiding this comment.
I also suggest sticking to the units that we expect it to be expressed in. In case the default is too cumbersome to express in seconds, then I'd change the units to something that better matches the default. Otherwise express the default in seconds.
| :returns: return if step should clean old repodata or not | ||
| :rtype: bool | ||
| """ | ||
| return not self.get_config().get('remove_old_repodata', True) |
There was a problem hiding this comment.
It looks like it's going to remove old metadata by default. Is that right?
There was a problem hiding this comment.
ok cool, ty I see the docs you wrote also reflect that so we're good there.
docs/tech-reference/yum-plugins.rst
Outdated
|
|
||
| ``remove_old_repodata_threshold`` | ||
| If ``remove_old_repodata`` is true, this attribute specify maximal age of | ||
| repodata in seconds that are not removed during publish. Thisttribute defaults |
There was a problem hiding this comment.
s/Thisttribute/This attribute/
| @@ -10,3 +10,6 @@ New Features | |||
|
|
|||
| * New parameter for rsync distributor ``rsync_extra_args`` allows user to | |||
| specify custom arguments for rsync call during publish operations. | |||
|
|
|||
| * New publish step RemoveOldRepodataStep removes repodata older than 14 days | |||
There was a problem hiding this comment.
Can a sentence be added here with links to the options saying something like: Configure this behavior with these options and link to the options.
There was a problem hiding this comment.
i don't think this is necessary, people know where to look for distributor's options docs.
There was a problem hiding this comment.
This can be skipped if you want. I was thinking it would just make the docs more awesome.
|
@midnightercz please address the comments once you have time, we are looking forward to see this merged. Thank you for collaboration and contributions 🎉 🎉 |
|
Also since this missed the 2.14 release branch point, master is now 2.15. Because of this, the release notes in this PR need to be updated to move from |
New publish step removes repodata older than threshold which is by default 14 days. closes pulp#2788 https://pulp.plan.io/issues/2788
There was a problem hiding this comment.
@midnightercz thanks for all the changes. This looks good to me.
|
@ipanova Would you be willing to merge whenever you think its ready? |
New publish step removes repodata older than threshold which is by default 14 days.
closes #2788
https://pulp.plan.io/issues/2788