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

Support multiple packages #72

Open
wants to merge 16 commits into
base: main
Choose a base branch
from
Open

Support multiple packages #72

wants to merge 16 commits into from

Conversation

OmarCastro
Copy link

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.

@OmarCastro OmarCastro requested a review from a team as a code owner April 4, 2022 15:44
@NamrataJha
Copy link
Contributor

Thanks @OmarCastro for the PR. Will take a look in the week of 18 April.

@carlos-m-rodrigues
Copy link

@NamrataJha is there any update?

@ivanaguilario
Copy link

I feel like this is a very important feature. Any updates @NamrataJha ?

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.

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 -
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: or package-names

Copy link
Author

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)
Copy link
Contributor

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?

Copy link
Author

@OmarCastro OmarCastro May 31, 2022

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.

@@ -0,0 +1,20 @@
/* eslint-disable @typescript-eslint/no-unused-vars */
Copy link
Contributor

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.

Copy link
Author

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?

Copy link
Contributor

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.

Copy link
Author

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
@NamrataJha
Copy link
Contributor

Also I am still not able to run the action. I am using this workflow for testing. The action is not able to fetch package-version-ids.

image

Also you will need to add package-names entry here in this action.yml file to remove the runtime warning.

@OmarCastro
Copy link
Author

Also I am still not able to run the action. I am using this workflow for testing. The action is not able to fetch package-version-ids.

image

Also you will need to add package-names entry here in this action.yml file to remove the runtime warning.

After analyzing action.yml and package.json, I noticed I have to run the pack script instead of build before commit, so the script was outdated

After that, all new commits now have dist/index.jsupdated

@OmarCastro
Copy link
Author

I have been testing this feature with success using this workflow

image

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

@carlos-m-rodrigues
Copy link

@NamrataJha Is there any update on this?

@nishthaGupta
Copy link
Contributor

nishthaGupta commented Jan 5, 2023

Hello @OmarCastro

We have released a major update for this action. See the release here. Could you please rebase your changes against main and submit the PR again?

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.

None yet

5 participants