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
Helm release lifecycle management endpoints #4580
Helm release lifecycle management endpoints #4580
Conversation
/test analyze |
So, Upgrade API should expect
So, in case of rollback, no communication with the chart repo is necessary.
Let's ensure the APIs are serving the needs of the UI. Please validate with @rohitkrai03 too. |
/retitle APIs for modification of a helm release |
/retitle [WIP] APIs for modification of a helm release |
Need to document the additional endpoints. |
/retitle [WIP] Helm release lifecycle management endpoints |
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.
missing handler uninstall tests.
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.
there are still a dozen of remarks that were not addressed.
Hey @akashshinde can you rebase your PR #4580 with master? It doesnt have the latest code for helm proxy and hence doesnt return the helm charts. |
863db03
to
e6ed240
Compare
/assign |
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.
please update the documentation as well.
/assign |
|
||
`ns=[string]` - Namespace | ||
|
||
`name=[string]` - Helm Release Name |
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.
URL Param for release name is not consistent across all API endpoints. /api/helm/release
endpoint needs URL param as release_name
. Can you also update that endpoint to be consistent?
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 would like to create a separate PR that could go into the code quicker. @akashshinde
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.
@rohitkrai03 the idea is to use name
instead of release_name
, that is the reason you would see name
param in all endpoints implemented in this PR. Going forward we will be renaming other endpoints url param from release_name
to name
in separate PR.
4a05477
to
e2f44f2
Compare
cc: @benjaminapetersen Please review this |
/lgtm |
if err != nil { | ||
// if there is no release exist then return generic error | ||
if strings.Contains(err.Error(), "no revision for release") { | ||
return nil, ErrReleaseRevisionNotFound |
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.
Not entirely sure this extra bit of logic helps?
no revision for release
revision not found for provided release
Pretty close to the same?
|
||
func RollbackRelease(releaseName string, revision int, conf *action.Configuration) (*release.Release, error) { | ||
if revision <= 0 { | ||
return nil, errors.New("Revision no. should be more than 0") |
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.
Nit, but I would prob use number
vs no.
rel, err := GetRelease(name, conf) | ||
if err != nil { | ||
// if there is no release exist then return generic error | ||
if strings.Contains(err.Error(), "no revision for release") { |
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.
Curious why a generic error, less information?
/approve A few questions on the errors, but I won't hold on that. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: akashshinde, benjaminapetersen, pedjak The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
This PR adds following endpoints
Release Details Response
Following things are pending.