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

Return the repo name in a carvel a pkg summary #4716

Merged
merged 9 commits into from
May 19, 2022

Conversation

antgamdia
Copy link
Contributor

Description of the change

As per #4543, it is now possible to populate the chip with the repo name of a carvel package. This PR just replicates the current approach for the Helm/Flux packages, that is, it does not introduces any big change with regards to how repos are referenced.
IMO, it should be carried out in a separate PR and leveraging the new Repo API.

Benefits

Carvel packages will also have a repo name, like the packages from Helm. Example:

image

Possible drawbacks

It has been implemented just returning Name: repo/pkgName, for instance: Name: tce-repo/grafana in the AvailablePackageSummary response, which is not consistent with the rest of the packages.
However, since we are going to refactor it as soon as we start integrating the new Repos APIs, I don't think it is such a big deal, but willing to hear your opinion.

Applicable issues

Additional information

Still a draft as I haven't written/adapted the tests yet.

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
@absoludity
Copy link
Contributor

This PR just replicates the current approach for the Helm/Flux packages, that is, it does not introduces any big change with regards to how repos are referenced. IMO, it should be carried out in a separate PR and leveraging the new Repo API.

+1 for postponing a longer term solution using the new Repo API, but why not kill two birds with one stone and use the current approach in the Helm/Flux packages to encode it in the AvailablePackageReference.identifier, rather than the AvailablePackageSummary.Name field?

It was done that way in the other plugins for two reasons:

  1. We needed a way to ensure the data was included in the response to the client (same purpose as here), and importantly,
  2. It moves closer towards an AvailablePackageReference.identifier that is unique for a plugin (I say moves closer, because with Helm's and Carvels support of per-namespace repositories, even my-repo/apache` may not uniquely identify the available package).

Let me know if there's a reason you chose to encode it into the AvailablePackageSummary.Name field instead (also, wouldn't that make the name inconsistent with the same name in the AvailablePackageDetail.Name?). Otherwise, I'd be keen that we encode the repo name in the AvailablePackageReference.identifier, improving that field too?

It has been implemented just returning Name: repo/pkgName, for instance: Name: tce-repo/grafana in the AvailablePackageSummary response, which is not consistent with the rest of the packages.

Just keen to understand why not do it consistently with the other plugins (using the AvailablePackageReference.Identifier) - it's just as simple, but also improves that field as above.

Thanks either way!

antgamdia added 3 commits May 17, 2022 16:23
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
@netlify
Copy link

netlify bot commented May 17, 2022

Deploy Preview for kubeapps-dev canceled.

Name Link
🔨 Latest commit f1307b1
🔍 Latest deploy log https://app.netlify.com/sites/kubeapps-dev/deploys/62856aae088b5a00087c74a0

@antgamdia
Copy link
Contributor Author

Let me know if there's a reason you chose to encode it into the AvailablePackageSummary.Name field instead
(...)
Just keen to understand why not do it consistently with the other plugins (using the AvailablePackageReference.Identifier) - it's just as simple, but also improves that field as above.

In fact, it was my first option, but then I noticed it would change the id we are requesting the pkg details with. Honestly, I wasn't aware (meaning I had forgotten it 😅 ) we wanted to move towards an FQDN with plugin and repo as the identifier, but the opposite (I thought we wanted to get rid of the repo for the helm/flux pkgs).

However, I'm 100% for using this FQDN as the id, so I have just pushed those changes (still missing tests, will address them soon, though)

if len(chartIDParts) != 2 {
return "", status.Errorf(codes.InvalidArgument, "Incorrect package ref dentifier, currently just 'foo/bar' patterns are supported: %s", chartID)
packageIDParts := strings.Split(unescapedPackageID, "/")
if len(packageIDParts) != 2 {
Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW (not for this PR), but this is the code that is very much related to #4284

antgamdia added 5 commits May 18, 2022 23:18
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
@antgamdia antgamdia marked this pull request as ready for review May 18, 2022 22:33
Copy link
Contributor

@absoludity absoludity left a comment

Choose a reason for hiding this comment

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

Excellent, thanks

@antgamdia antgamdia merged commit 1717a7d into vmware-tanzu:main May 19, 2022
@antgamdia antgamdia deleted the 4543-addCarvelRepos branch May 19, 2022 12:27
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.

[carvel] Add pkg "repository" to the packges
2 participants