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

New CLI Flag --devel To Include Development/Prerelease Versions of Charts #139

Merged
merged 2 commits into from
Feb 11, 2023

Conversation

Bhargav-InfraCloud
Copy link
Contributor

@Bhargav-InfraCloud Bhargav-InfraCloud commented Dec 18, 2022

Motivation

Issue #94

Changes

  1. New CLI flag --devel.
  2. Tests to cover Charts() and Containing().

Summary

CLI flag --devel to list/include development (prerelease versions like release candidate, alpha, beta, etc.) in the following endpoints:

  • /api/helm/repositories/<repo>
  • /api/helm/repositories/versions?name=<repo>
  • /api/helm/repositories/latestver?name=<repo>

Related Issues

Closes #94

@undera
Copy link
Collaborator

undera commented Dec 19, 2022

There is massive refactoring happens in #131 , will have to wait for it to complete

@undera
Copy link
Collaborator

undera commented Jan 30, 2023

Just to record my investigations, exerpt from Helm sources, search_repo.go:

if o.devel { // search for releases and prereleases (alpha, beta, and release candidate releases).
		o.version = ">0.0.0-0"
	} else { // search only for stable releases, prerelease versions will be skip
		o.version = ">0.0.0"
	}
constraint, err := semver.NewConstraint(o.version)
v, err := semver.NewVersion(r.Chart.Version)
if constraint.Check(v) {
}

@undera
Copy link
Collaborator

undera commented Jan 30, 2023

The proper approach to this feature would include the checkbox in UI to enable display of development charts, passing that as parameter to API

@Bhargav-InfraCloud
Copy link
Contributor Author

@undera Thanks for the pointers. It is working as expected. A query before submitting this PR:

How does the user enable development versions:

  1. Directly from UI, the API response for the backend?
  2. Starts the application using --devel which enables the option on UI to toggle on/off development versions, which is then sent to the backend as API response?

@undera
Copy link
Collaborator

undera commented Feb 6, 2023

Two factors I take into account:

  1. Seems that not too many users use devel versions in Helm
  2. It would be harder to add a UI checkbox to install/upgrade modal for versions filter.

From that, I would do it as an environment variable. Once set, the user would get versions filtered accordingly.

Note that the filtering has to apply not only in Charts(), but also for ByName()

@Bhargav-InfraCloud
Copy link
Contributor Author

Bhargav-InfraCloud commented Feb 7, 2023

@undera Thanks! That makes sense. Please confirm the following:

  1. In Charts(), the current logic is only considering a single version (0'th index) of each chart. Any particular reason in doing so or should my changes iterate over all versions, filter, and append?

  2. From that, I would do it as an environment variable. Once set, the user would get versions filtered accordingly.

    If the devel is enabled/disabled only once from environment variables, why not have the --devel flag instead, as we initially discussed?

@undera
Copy link
Collaborator

undera commented Feb 7, 2023

  1. The purpose of Charts() is to return the list of all charts, with the latest version specified. So, if the chart only has dev versions, we don't display it when flag is not set. We can't return all the versions 'cause it will produce too much output.

  2. You can add that flag. Just it will be harder to propagate it to all the objects. If that flag is rarely used, it might not be that important. Up to you here.

@Bhargav-InfraCloud
Copy link
Contributor Author

@undera While I was trying to add tests for these changes, I noticed that they need interfaces for mocking. Otherwise, not testable.

However, when I started adding interfaces and tests, I realized that it is reorganizing most parts or pkg/dashboard/objects. So, I reverted those extra changes. I'll submit them under #210 if approved.

This card can only be tested manually. These are the sample calls I used to verify, with and without --devel. Please check and let me know.

curl http://localhost:8080/api/helm/repositories/kafka-operator | jq '.[].version'
curl http://localhost:8080/api/helm/repositories/versions\?name\=kafka-operator | jq '.[].version'
curl http://localhost:8080/api/helm/repositories/latestver\?name\=kafka-operator

@Bhargav-InfraCloud Bhargav-InfraCloud marked this pull request as ready for review February 8, 2023 05:12
go.mod Outdated Show resolved Hide resolved
@undera
Copy link
Collaborator

undera commented Feb 8, 2023

I have replied in #210 about testing. The code is testable, with the Helm's standard way of injecting fixtures. I don't see a real need in in introducing interfaces.

The flag `--devel` for enabling/disabling dev versions
of charts in following endpoints:
1. /api/helm/repositories/kafka-operator
2. /api/helm/repositories/versions
3. /api/helm/repositories/latestver

Signed-off-by: Bhargav Ravuri <bhargav.ravuri@infracloud.io>
@undera
Copy link
Collaborator

undera commented Feb 11, 2023

Do you plan to add the test @Bhargav-InfraCloud ?

Signed-off-by: Bhargav Ravuri <bhargav.ravuri@infracloud.io>
@Bhargav-InfraCloud Bhargav-InfraCloud changed the title Add --devel Flag for Functions Called by Repository Tab New CLI Flag --devel To Include Development/Prerelease Versions of Charts Feb 11, 2023
@Bhargav-InfraCloud
Copy link
Contributor Author

@undera My apologies, I didn't find time to update you on the progress. I've submitted the commit with test cases. Please review.

@undera
Copy link
Collaborator

undera commented Feb 11, 2023

Looks good. Awesome contributioN!

@undera undera merged commit 6a4ca79 into komodorio:main Feb 11, 2023
@Bhargav-InfraCloud Bhargav-InfraCloud deleted the devel-flag branch February 11, 2023 19:26
@Bhargav-InfraCloud
Copy link
Contributor Author

Bhargav-InfraCloud commented Feb 11, 2023

Thanks, @undera. I appreciate the help throughout the time. 😊

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.

[Feature request] Repository tab support to display helm that only have pre-release versions
2 participants