Update retention-rules.mdx to align with proper working functionality based on Engineering feedback. #295
Conversation
… based on Engineering feedback.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull request overview
This PR updates the retention rules documentation to align with actual functionality based on engineering feedback. The changes improve accuracy, clarity, and structure of the documentation, particularly regarding how conditions are applied, the evaluation sequence, and the Limit by Size parameter behavior.
Changes:
- Restructured and expanded documentation to better explain retention rule functionality and evaluation sequence
- Added detailed explanation of when retention rules are evaluated (upload vs resync triggers)
- Clarified how the Limit by Size parameter orders packages by semantic version (not upload date)
- Fixed grammatical issues and improved consistency in capitalization and formatting
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@jtaylor-cs I've opened a new pull request, #297, to work on those changes. Once the pull request is ready, I'll request review from you. |
Address resolved feedback - no new comments to action
[WIP] Update retention-rules.mdx based on PR #295 feedback
|
The only thing I cannot tell is whether the formatting of the is okay. The internet states that GitHub's Markdown preview does not support this MDX attribute, so it does not format correctly in the Preview. |
|
DM'd link to preview to verify formatting |
ralph-mcteggart
left a comment
ralph-mcteggart
left a comment
There was a problem hiding this comment.
Looks good just a small styling comment.
I can't approve on the content just cause I'd need to go find out how it all works again! @chrisimcevoy might know offhand to verify there
Fixed some double-quoting and wrapping of numerical values in parentheses.
jtaylor-cs
left a comment
jtaylor-cs
left a comment
There was a problem hiding this comment.
Made some minor adjustments to formatting.
|
@dmeecs, would you be willing to review this content for functional accuracy, since you were the one to clarify how conditions were handled, you will probably have the best insight. |
|
There is a group effort to rewrite this entire document. We may reuse this existing PR. For now, it will remain open until we determine how to proceed. Note: Even some of the function I have refined is still incorrect. This is now a joint effort with Product, Engineering, etc. |
| ### Limiting the number of packages to delete | ||
| The **Limit by count** option defines the number of packages to keep. For example, if we set its value to `4` and only a total of `3` packages meet the criteria, then `0` packages will be deleted. But if `5` packages meet the criteria, then `1` will be deleted and `4` will be keep in the repository. | ||
| ### Understanding the Sequence of an Evaluation | ||
| 1. **Initial Query Set is Created** - All successfully synced packages within the repository are identified. |
There was a problem hiding this comment.
Let's not talk about 'Query Set'. It's leaking our internal implementation information to an external document.
Instead let's call this a 'Group'. It's a more agnostic term.
| | Limit by Days | `retention_days_limit` | The number of days to retain packages. A cutoff date is calculated, and packages with an upload date before this cutoff date are selected for deletion; packages uploaded on or after the cutoff date are retained. Set to zero to remove this criterion from the rule. | | ||
| | Limit by Count | `retention_count_limit` | The maximum number of packages to retain. Set to zero to remove this criterion from the rule. | | ||
| | Limit by Size | `retention_size_limit` | The maximum total size (in bytes) of packages to retain. Set to zero to remove this criterion from the rule. | | ||
| | Group Packages by Name | `retention_group_by_name` | If enabled, retention will apply to groups of packages by name rather than all packages. For example, when `retention_count_limit` is defined as "1" and packages `PkgA 1.0`, `PkgB 1.0`, and `PkgB 1.1` are identified as eligible for deletion; only `PkgB 1.0` is deleted because there are (2) `PkgBs` and (1) `PkgA`; this parameter is applied to each grouped package name. | |
There was a problem hiding this comment.
I find the example written for these parameters confusing. e.g. these bits
For example, when `retention_count_limit` is defined as "1" and packages `PkgA 1.0`, `PkgB 1.0`, and `PkgB 1.1` are identified as eligible for deletion; only `PkgB 1.0` is deleted because there are (2) `PkgBs` and (1) `PkgA`; this parameter is applied to each grouped package name.
The first bit If enabled, retention will apply to groups of packages by name rather than all packages. is sufficient for this section.
I agree it's important to describe how the grouping works. Absolutely. But it's better to put that in its own section below, as you've done. To describe what is happening.
I would probably extend your Understanding Groupings to include examples for the different combinations of groups, if that is necessary.
| 1. **Initial Query Set is Created** - All successfully synced packages within the repository are identified. | ||
| 2. **Grouping is Applied to the Initial Query Set** - The initial query set of all packages is grouped by format, grouped by name, and grouped by type based on the enabled parameters specified in the retention rule. To be clear, if all 3 grouping parameters are enabled, this will result in 3 separate query sets. | ||
| 3. **Package Query String is Applied to Query Set** - The query string filters the packages of each existing query set. Again, if multiple grouping parameters are enabled, the query string is applied to each query set separately. | ||
| 4. **Query Set is Ordered by Package Upload Date** - Any query set(s) gets sorted based on the upload date of the packages, newest to oldest, and excluding the newest package used to trigger the evaluation. |
There was a problem hiding this comment.
While true, this is to much like the internal implementation.
Instead, each limit by Count, limit by days, limit by size, should have a sentence saying for that limit how the deletes are ordered.
|
|
||
| ### Limiting the number of packages to delete | ||
| The **Limit by count** option defines the number of packages to keep. For example, if we set its value to `4` and only a total of `3` packages meet the criteria, then `0` packages will be deleted. But if `5` packages meet the criteria, then `1` will be deleted and `4` will be keep in the repository. | ||
| ### Understanding the Sequence of an Evaluation |
There was a problem hiding this comment.
I'd probably rename this as 'How limits and groups create rich retention rules`
I know, it's a bit like sales language, that's basically what's happening.
Plus, I'd put this after the When Do Retention Rules Get Evaluated? section.
I'd also consider removing the Understanding Groupings section. As I think this section basically explains it perfectly, plus with the examples, that's pretty good.
And I'd put the Other Considerations just as a paragraph in this section.
I'd split this section into two headings really
# How groups are formed
... <stuff about groups>
# How limits are applied
When multiple parameters of a retention rule are enabled, a package that meets any of the enabled conditions (Limit by Count, Limit by Days, Limit by Size) will be deleted.
... stuff about limits
Based on feedback from ENG-10496, there were parts of the documentation that were inaccurate about how conditions were applied.
Based on other feedback and a previous pending PR #291, I updated the structure of the documentation to ellaborate on key points and further clarify functionality.
Added important details about Limit by Size function, based on ENG-10292. Query set is order based on semantic version, not Upload Date as with the other days and count parameters.
Fixed various grammatical issues that were detected.