Skip to content
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 support for deleting packages on new GitHub Packages architecture #83

Merged
merged 26 commits into from
Jan 5, 2023

Conversation

s-anupam
Copy link
Contributor

Overview

Package registries on the new GitHub Packages architecture, including container registry, npm and nuget packages, no longer expose data through the GraphQL API. This action currently relies on using GraphQL to get and delete versions, thus making it incompatible with the new architecture

Note
Deprecation Notice: GraphQL for Packages

In this PR, we replace the GraphQL dependency with GitHub REST API for Packages along with some other changes and improvements

Changes

  • GraphQL calls replaced with REST API calls using @octokit/rest package
  • Remove batch processing of version deletes. This fixes a bug that could lead to deleting more versions than ideal when min-versions-to-keep is given. Now all versions are first fetched and then processed in a single batch
  • Better unit tests coverage

Changes in action parameters

  • Add package-type param which is required in the REST APIs
  • Remove repo param as it is no longer needed
  • Make package-name param required even if package-version-ids is specified

Related

@s-anupam s-anupam marked this pull request as ready for review January 3, 2023 04:33
@s-anupam s-anupam requested a review from a team as a code owner January 3, 2023 04:33
expect(result.versions.length).toBe(numVersions)
done()
})
const RATE_LIMIT = 99
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The RATE_LIMIT was a limitation due to limitation of graphql api. Do we have the same limitation with REST or can it be updated/removed?

Copy link
Contributor

@nishthaGupta nishthaGupta Jan 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The delete version rest API works for a single version deletion only; hence no concept of rate-limiting here. However, I still think that we should rate-limit deletions in a single workflow. We can decide on the limit value.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rate limit is 1000 requests per repository per hour when using GITHUB_TOKEN and 5000 requests per user per hour when using PAT. Considering the limit with GITHUB_TOKEN, I think 100 is sufficient as we would not want this workflow to exhaust a significant part of limit and possibly cause issue with other workflows.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to 100 - 2de4236

src/delete.ts Outdated
new Date(a.created_at).getTime() - new Date(b.created_at).getTime()
)
})
/*
Here first filter out the versions that are to be ignored.
Then update input.numOldeVersionsToDelete to the no of versions deleted from the next 100 versions batch.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we remove the batch part from the comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed the comment in 2de4236

action.yml Outdated

package-type:
description: >
Type of the package containing the version to delete.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: the language here can be clearer since this is a customer facing help string for the param

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to 👇 (2de4236)

Type of package. Can be one of container, maven, npm, nuget, or rubygems.

suyash691
suyash691 previously approved these changes Jan 4, 2023
NamrataJha
NamrataJha previously approved these changes Jan 4, 2023
@nishthaGupta nishthaGupta dismissed stale reviews from NamrataJha and suyash691 via 4833470 January 5, 2023 08:35
Copy link
Contributor

@NamrataJha NamrataJha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

@s-anupam s-anupam merged commit fe2a95a into main Jan 5, 2023
@s-anupam s-anupam deleted the s-anupam-rest-api branch February 28, 2023 08:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for deletion of container and npm packages
4 participants