-
Notifications
You must be signed in to change notification settings - Fork 558
feat: Add ability to Link external helm app with devtron chart #1320
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
Conversation
| InstallApp(w http.ResponseWriter, r *http.Request) | ||
| GetInstalledAppsByAppStoreId(w http.ResponseWriter, r *http.Request) | ||
| DeleteInstalledApp(w http.ResponseWriter, r *http.Request) | ||
| UpdateHelmApplicationWithChartStoreLinking(w http.ResponseWriter, r *http.Request) |
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.
better naming.
for already linked app update are we calling same method?
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.
name refactored.
For already app, we are not calling the same method. currently, update is happening externally to helm directly. we have to update link also. this item we have in our pipeline.
| //RBAC enforcer Ends | ||
|
|
||
| // check if chart repo is active starts | ||
| isChartRepoActive, err := handler.appStoreDeploymentService.IsChartRepoActive(int(request.GetAppStoreApplicationVersionId())) |
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.
move this to service layer
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.
done
| return nil, err | ||
| } | ||
|
|
||
| var appInstallationMode string |
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 can directly assign the variable instead of if else
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.
if else is needed due to installAppVersionRequest.AppOfferingMode check in if condition
Fix of #1270
Description
Functionality added for :
Fixes #1270
Type of change
How Has This Been Tested?
By updating the helm App release with linking (API invoking)
Checklist: