-
Notifications
You must be signed in to change notification settings - Fork 86
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
Support multiple packages #72
base: main
Are you sure you want to change the base?
Conversation
Thanks @OmarCastro for the PR. Will take a look in the week of 18 April. |
@NamrataJha is there any update? |
I feel like this is a very important feature. Any updates @NamrataJha ? |
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.
Apologies for missing the date here. @OmarCastro I tried testing this feature, but the action errors out and is not able to get the pakcage-versions-ids
for deletion.
Can you provide a sample workflow where you have tested this?
Have left some comments.
README.md
Outdated
@@ -70,7 +81,7 @@ This action deletes versions of a package from [GitHub Packages](https://github. | |||
|
|||
# Valid Input Combinations | |||
|
|||
`owner`, `repo`, `package-name` and `token` can be used with the following combinations in a workflow - | |||
`owner`, `repo`, `package-name` (of `package-names`) and `token` can be used with the following combinations in a workflow - |
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.
typo: or package-names
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.
fixed, thank you
src/delete.ts
Outdated
: EMPTY | ||
), | ||
tap( | ||
value => (totalCount = totalCount === 0 ? value.totalCount : totalCount) |
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.
we use the totalCount variable to keep a check of no of total versions for a package. Using the same variable here will cause version count to be faulty.
Also what is the requirement for getting total no of packages?
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 had to review the whole code to see why that line was there. It is a bad idea to add that line since as you say, it is already being used for another purpose. I removed it, and because totalCount is no longer used, I removed all references on src/packages
since there is no need to get the total number of packages.
src/packages/graphql.ts
Outdated
@@ -0,0 +1,20 @@ | |||
/* eslint-disable @typescript-eslint/no-unused-vars */ |
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 file is same as the one in versions folder. We can reuse the code here.
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, we can. I duplicated this file since I could not find how are common features organized in other Github actions projects.
So, to help to solve this little issue, I ask... How do you organize common features? Create a common
folder?
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 we can move it inside a new common
folder.
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.
finished the refactor to a common
folder
there is no need to get totalCount if it is not going to be used - remove faulty tap call on getPackageNames method - remove totalCount on query, as pageInfo.hasNextPage is enough for pagination - remove totalCount on graphql.mock.ts
Also I am still not able to run the action. I am using this workflow for testing. The action is not able to fetch Also you will need to add |
- move graphql.ts to a common folder - update imports - remove duplicate graphql.ts
After analyzing action.yml and package.json, I noticed I have to run the After that, all new commits now have |
I have been testing this feature with success using this workflow One thing I noticed is the minimal information on the logs, only total number of versions deleted. With this feature implemented, I propose to add the number of versions removed by package |
@NamrataJha Is there any update on this? |
Hello @OmarCastro We have released a major update for this action. See the release here. Could you please rebase your changes against |
This PR adds support for multiple packages, a requested feature in #65. A new field
package-names
that supports this feature was added to make the code backward compatible.