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.
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
Add a separate action for removing old wheels #95
base: main
Are you sure you want to change the base?
Add a separate action for removing old wheels #95
Changes from 22 commits
d6264b8
9d4f667
002d2e5
40cd6bc
8a33f20
b18b4a1
deb3cba
05770f3
199b1c8
f8b96a0
70f8a3d
2fac172
6a1b65e
8bb7bb6
bae5ad6
f916adc
afedc16
35c9d59
7a7dbbc
7830eae
2f62d85
1f04be6
b6dbd44
88ce687
03979b0
bf8e993
433e7c3
4a3497d
71ede79
2d2e62a
556e9a6
e403c81
15b617e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
This is called organization, not channel; are these interchangeable terms? If so, use channel; if not, explain the difference.
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'm not quite sure, since they look like they have been used interchangeably. For example, the README outside of this PR:
upload-nightly-action/README.md
Lines 48 to 67 in 920fb59
mentions how one may upload to a different channel, but it uses "organization" as an input.
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 am not sure if the Anaconda token that is authorised to maintainers of, say, a package X is restricted to only remove wheels for said package X, or whether it can remove all packages in the index. This is because while users of SPNW won't be affected since we handle the deletions, users with their own organisation with wheels for packages X, Y, and Z being uploaded to it would either:
Which situation would be more plausible? If it is the latter, we'd have to provide another input in the action for a comma-separated string of packages to delete uploads for (and maybe
*
for deleting all packages?). To replace the existence ofpackages-ignore-from-cleanup.txt
(which would exist only in this repository – see below), it might also make sense to include a whitelist input, too. If it's the former, it gets easier to implement, but users won't have fine-grained control on automations for what packages are being deleted.P.S. This is all valid only if I'm not missing something about how the index and its permissions are structured :) Happy to receive others' thoughts!
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.
Based on @tupui's suggestion in #95 (comment), users will need to run the action multiple times to remove old uploads for multiple packages. Hence, the second option of allowing per-package deletions is better, and an input for a list of packages to delete wheels for isn't required.
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.
However, if someone has an appropriately scoped token for their organisation, they can very well remove multiple wheels from the index at a time, and some users might want to do that. So, it might be necessary to mention in the documentation how the tokens work and pre-emptively warn about possible deletions in an admonition.
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.
To be clear, users have full control over their project. So they can add or remove any wheel they want on that project. People can just not add members to their project and they need to ask admins.
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.
Please see above for context and my question on this. :)
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've put some thought into it, and this can be included as an input in the action. There would be two cases:
While the former would be recommended to limit deletion scopes, when the latter is used by the action users (as it is being done here for this repo), a
whitelist_packages:
input can be added to include a comma-separated list of packages ("openblas-libs"
in our case).Another reasonable message could be to add a warning:
"Wheels for package X requested for deletion, but X is whitelisted. Please either remove it from the whitelist or try to delete a different package as needed."
.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.
This TODO item would also be resolved if we have a way forward with the questions above, since, if multiple packages are removed at once, this loop can stay; if we were to restrict the action to removing just one wheel at a time (which I don't think we should), then we could remove this. I think an action of the form:
is better than specifying the step and authenticating multiple times.
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.
packages: ["a", "b", "c"]
hopefully!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.
Yes, a list of strings sounds elegant! :)