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 credential info to repository summary #5003

Merged
merged 9 commits into from
Jul 7, 2022

Conversation

antgamdia
Copy link
Contributor

Description of the change

This PR adds a slight modification to the Repos APIs: the GetPackageRepositorySummaries now supports an additional auth field. The proposed semantic is:

  1. summary.auth == nil then the repo doesn't have any credentials.
  2. summary.auth == &corev1.PackageRepositoryAuth{} then the repo has some sort of credentials to access it.

For saving time and k8s requests, is enough to send a non-nil value. However, it can also be turned into a boolean field, but wanted to open up the discussion with you all first.

Benefits

The UI will be able to display this info without fetching the details of each repo, which could be a lengthy operation (not in this PR though, this is just a modification in the API).

Possible drawbacks

N/A

Applicable issues

Additional information

image

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>
@netlify
Copy link

netlify bot commented Jul 1, 2022

Deploy Preview for kubeapps-dev canceled.

Built without sensitive environment variables

Name Link
🔨 Latest commit c05da9a
🔍 Latest deploy log https://app.netlify.com/sites/kubeapps-dev/deploys/62c5be568d8f980009f8db3a

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

The access level is a very useful information for a summary, thanks for the work!

I have my doubts regarding the type used, why PackageRepositoryAuth? The same goal could be achieved with a flag, or a string/enum containing different levels of access ("private", "public", or others if any).
Auth information itself (type, header, keys, etc) looks like a detailed data of the repo, not something to be shown in the summary.

@antgamdia
Copy link
Contributor Author

I have my doubts regarding the type used, why PackageRepositoryAuth? The same goal could be achieved with a flag, or a string/enum containing different levels of access ("private", "public", or others if any).

I agree, there is no need to use the PackageRepositoryAuth. I went this way because I didn't want to make the summaries response differ so much from the detail resp. So using the same auth seemed fair enough. However, I agree, plugins won't be setting the PackageRepositoryAuth properties and it just becomes a boolean field.
Besides, with the implementation divergence, we have noticed in the auth header types, we better restrict the implementation possibilities.

What about a hasAuth field? I wouldn't want to add more terminology to the already existing one (hence avoiding terms like "accessLevel" or "isPublic"). WDYT ? I'm gonna start performing the changes while we come up with a good name for the field (it is always the most difficult part, hehe)

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

Conflicts:
	cmd/kubeapps-apis/plugins/helm/packages/v1alpha1/repositories.go
	cmd/kubeapps-apis/plugins/helm/packages/v1alpha1/repositories_test.go
@castelblanque
Copy link
Collaborator

+1 to hasAuth field flag approach.
Alternative name idea: requiresAuth.
Thanks!

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

@castelblanque castelblanque left a comment

Choose a reason for hiding this comment

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

Awesome, thanks!

@antgamdia antgamdia merged commit 45ce686 into vmware-tanzu:main Jul 7, 2022
@antgamdia antgamdia deleted the 4866-authInRepoSummary branch July 7, 2022 10:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Repository] add credential info to repository summary
3 participants