-
Notifications
You must be signed in to change notification settings - Fork 95
Add option to exclude package names from deletion #10
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
Closed
Closed
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Hsn723 did you look into a solution that exclude the versions from the query in the first place? If that is even possible.
Could you run into a situation where there'll never be any versions left. The case where
numOldVersionsToDeleteis smaller than the number of versions matching you regular expression? Would one have to "know your versions" and set this value higher to avoid that or what do you think?Kind of related: Do you know if this action could remove all versions or will it at least leave one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did look into passing the filter into the query however the current GitHub GraphQL API v4 does not support any filtering at this time.
Indeed, there's the possibility of the oldest
numOldVersionsToDeleteversions all match the filtering pattern and therefore ending up with nothing to delete. Unfortunately I don't have an elegant solution other than "know your versions" as you mentioned. Or, having a versioning scheme that allows some predictability. Automatically counting up is also an option, however that will require several passes.Yes, even with the initial implementation this action can very well remove all versions, for instance if
numOldVersionsToDeleteis set higher to the total number of packages. Leaving at least one (the latest version) would indeed be desirable, although probably out of the scope of the current PR (I can gladly prepare another PR for that though).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your valuable replies. I can see the complexities.
I find it odd that the default implementation would let you delete all versions. I'm afraid to use the action without this "protection".
Also, the
numOldVersionsToDeleteis kind of up-side-down isn't it? Wouldn't it make more sense with akeepNumVersions+ what your PR providers?Yeah, that would be a different PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
keepNumVersionshas also been proposed in #5, also that one is also tricky with the current GitHub GraphQL API v4 as the API doesn't provide a way to return "all" package versions unless you call the API asking for the "first N packages" with a large enough number.By the way, if #5 gets implemented it would also be possible to combine it with the
ignore-package-namesproposed in this PR.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess any, not necessarily large, number would suffice. As long as you don't add more than whatever that number is. At each delete action execution it would carve away old versions until there are no more of them. Isn't that how it would/could work?
Good to know, thank you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In theory yes, for instance if you set
keepNumVersionsto 5 and call the API every time requesting the 10 latest versions. What could possibly betray user expectations however is that this way you would end up deleting old (but not oldest) packages and slowly working your way down when the user would rather have oldest packages deleted first. Though the reality is that automation with the current API has no "one size fits all" and some users' expectations may be betrayed regardless.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. Thanks for the explanation.