feat: Multi charts #1013

aviralsrivastava01 · 2021-12-20T09:16:56Z

Description

Added API for uploading templates directly

Fixes #1325
Allowing user to upload custom deployment templates

Type of change

[yes ] New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Changes are tested
Yes, checked uploading new charts, checked whether it rejects existing charts(name, version), checked by uploading file with missing files

Checklist:

[yes ] The title of the PR states what changed and the related issues number (used for the release note)
[yes] I have performed a self-review of my own code
[yes] I have tested it for all user roles

…into multi-charts

…1325]

pghildiyal · 2022-03-21T10:09:54Z

api/deployment/DeploymentRouter.go

@@ -0,0 +1,22 @@
+package deployment


file name should be DeploymentConfigRouter

pghildiyal · 2022-03-21T10:10:14Z

api/deployment/DeploymentRestHandler.go

@@ -0,0 +1,122 @@
+package deployment


file name should be DeploymentConfigRestHandler

pghildiyal · 2022-03-21T10:25:06Z

pkg/pipeline/ChartService.go

 	DeploymentTemplateValidate(templatejson interface{}, chartRefId int) (bool, error)
 	JsonSchemaExtractFromFile(chartRefId int) (map[string]interface{}, error)
 	GetSchemaAndReadmeForTemplateByChartRefId(chartRefId int) (schema []byte, readme []byte, err error)
+	ExtractChartIfMissing(ChartData []byte, RefChartDir string, Location string) (string, string, string, error)


name of arguments cant start with capital letter do it for all the functions added whereever applicable

pghildiyal · 2022-03-21T10:28:51Z

pkg/pipeline/ChartService.go

+
+	dir := impl.chartTemplateService.GetDir()
+
+	RandomChartWorkingDir := filepath.Join(RefChartDir, Location)


should not start with capital letter

and it may not be random

pghildiyal · 2022-03-21T10:35:15Z

api/deployment/DeploymentRestHandler.go

+		return
+	}
+
+	exists, err := handler.chartRefRepository.DataExists(chartName, chartVersion)


check this before extracting or atleast before copying otherwise this ll create issue, even though it is rejecting the upload it has already copied file in final destination

pghildiyal · 2022-03-21T10:36:16Z

api/router/router.go

 	r.pProfRouter.initPProfRouter(pProfListenerRouter)
+
+	//  deployment router starts
+	deploymentSubRouter := r.Router.PathPrefix("/orchestrator/deployment").Subrouter()


it is deployment/template

pghildiyal · 2022-03-21T10:38:07Z

specs/deployments.yaml

+        description: form-data as request body
+        required: true
+        content:
+          application/json:


content is not application/json

pghildiyal · 2022-03-21T10:39:18Z

util/helper.go

 }
+
+func CheckForMissingFiles(ChartLocation string) error {
+	listofFiles := [7]string{"app-values.yaml", "Chart.yaml", "env-values.yaml", "pipeline-values.yaml",


we had to check for both .yaml file as well as .yml file i dont see that here

…use [1325]

kartik-579 · 2022-03-22T11:01:25Z

api/deployment/DeploymentConfigRestHandler.go

+	"time"
+)
+
+type DeploymentRestHandler interface {


can we change interface and impl object from Deployment to DeploymentConfig, so that one does not gets it confused with deployment operation

kartik-579 · 2022-03-22T11:03:56Z

api/deployment/DeploymentConfigRestHandler.go

+	}
+
+	if err != nil {
+		common.WriteJsonResp(w, err, nil, http.StatusBadRequest)


please add log here

logging was already done in the function for each and every error so to avoid duplicate logging I ignored logging here

kartik-579 · 2022-03-22T11:04:45Z

api/deployment/DeploymentConfigRestHandler.go

+	err = handler.chartRefRepository.Save(chartRefs)
+	if err != nil {
+		handler.Logger.Errorw("error in saving ConfigMap, CallbackConfigMap", "err", err)
+		common.WriteJsonResp(w, err, "Chart couldn't be saved", http.StatusBadRequest)


this should be InternalServerError(500), right?

kartik-579 · 2022-03-22T11:05:25Z

api/deployment/DeploymentConfigRouter.go

+
+import "github.com/gorilla/mux"
+
+type DeploymentRouter interface {


update interface and impl in this file to DeploymentConfig as well

kartik-579 · 2022-03-22T11:06:13Z

api/router/router.go

 	r.pProfRouter.initPProfRouter(pProfListenerRouter)
+
+	//  deployment router starts
+	deploymentSubRouter := r.Router.PathPrefix("/orchestrator/deployment/template").Subrouter()


please update this to deploymentConfig as well

kartik-579 · 2022-03-22T11:08:58Z

pkg/chartRepo/repository/ChartRepoRepository.go

 	GetDefault() (*ChartRef, error)
 	FindById(id int) (*ChartRef, error)
 	GetAll() ([]*ChartRef, error)
+	DataExists(name string, version string) (bool, error)


can we change this method name to CheckIfDataExists or something like this for clearer understanding?

kartik-579 · 2022-03-22T11:11:48Z

pkg/pipeline/ChartService.go

+	ExtractChartIfMissing(chartData []byte, refChartDir string, location string) (*ChartDataInfo, error)
+	CheckChartExists(chartRefId int) error
+	GetChartLocation(chartDir string, fileName string) string
+	ValidateFileUploaded(fileName string) error


are we uploading any files in response?

kartik-579 · 2022-03-22T11:12:53Z

pkg/pipeline/ChartService.go

+	return chartLocation
+}
+
+func (impl *ChartServiceImpl) ValidateFileUploaded(fileName string) error {


this is for validating the received file in payload from user.. can we change the uploaded file part to avoid confusion

right it can be ValidateUploadedFile

kartik-579 · 2022-03-22T11:14:52Z

pkg/pipeline/ChartService.go

+	CheckChartExists(chartRefId int) error
+	GetChartLocation(chartDir string, fileName string) string
+	ValidateFileUploaded(fileName string) error
+	ReadChartYamlForLocation(chartDir string, fileName string) (string, string, error)


can update the names of the output data variables here instead of only showing the type

change the name of the function also, focus not on how but what. It is more like GetChartNameAndVersionForLocation

most of the functions are following the above format

main action is fetching chart metadat and not reading file, if tomorrow we start reading it from some other file its name should not change

this is pending

kartik-579 · 2022-03-22T11:17:35Z

pkg/pipeline/PropertiesConfig.go

+	var chartService ChartService
+	err := chartService.CheckChartExists(environmentProperties.ChartRefId)
+	if err != nil {
+		impl.logger.Errorw("error in getting missing chart for chartRefId", "err", err, "chartRefId")


please pass value for key chartRefId

pghildiyal · 2022-03-28T03:25:01Z

util/helper.go

+			}
+		}
+		if len(missingFiles) != 0 {
+			return errors.New("Missing files " + strings.Join(missingFiles, ",") + " files")


missing file name should be chart.yml or chart.yaml and not chart

pghildiyal · 2022-03-28T03:25:13Z

util/helper.go

+		".image_descriptor_template.json": true,
+		"chart":                           true,
+	}
+	missingFilesMap[".image_descriptor_template.json"] = true


this line is redundant

Aviral Srivastava and others added 30 commits December 3, 2021 11:59

sql query

5a97b7f

sql query

4e7e9a5

sql query

be7b0eb

sql query change

3658a1d

add name

b6b50f8

add name

eab44ac

Merge branch 'main' into multicharts

8d34bc4

chart-sync

f38098e

multi-charts

65f2adc

save-Configmaps

8288ce0

configmaps

64faf77

configmap-charts

e7169bb

schema-validate

1160056

schema-fix

351f923

main merge

68dd194

copy dir

dc257fb

minor changes

e8fcc30

Delete .helmignore

240008d

Delete 580

873a094

Delete .image_descriptor_template.json

8b53ce9

Delete Chart.yaml

91d0227

Delete values.yaml

98308d9

Delete kn-service.yaml

30d939f

Delete generic.yaml

0076d46

Delete release-values.yaml

62111f3

Delete env-values.yaml

b7654c4

Delete pipeline-values.yaml

620bee3

Delete app-values.yaml

9b0060d

fixes

98497b5

Merge branch 'multi-charts' of https://github.com/devtron-labs/devtron …

59c03ce

…into multi-charts

snehith57624 added 3 commits March 21, 2022 09:45

Merge branch 'main' into multi-charts

7d2f069

merge conflicts

22148ef

Adding return for failure and custom error message when data exists […

9518a99

…1325]

pghildiyal reviewed Mar 21, 2022

View reviewed changes

api/deployment/DeploymentRouter.go Outdated

@@ -0,0 +1,22 @@

package deployment

Copy link

Contributor

pghildiyal Mar 21, 2022

Choose a reason for hiding this comment

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

file name should be DeploymentConfigRouter

pghildiyal reviewed Mar 21, 2022

View reviewed changes

api/deployment/DeploymentRestHandler.go

@@ -0,0 +1,122 @@

package deployment

Copy link

Contributor

pghildiyal Mar 21, 2022

Choose a reason for hiding this comment

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

file name should be DeploymentConfigRestHandler

pghildiyal reviewed Mar 21, 2022

View reviewed changes

snehith57624 added 5 commits March 21, 2022 19:59

Updating changes as per PR comments, deleting temporary folder after …

abb5be5

…use [1325]

Refactoring deleting temporary folder logic for every error[1325][EA2]

b0e8737

Refactoring api arguments [1325][EA2]

a7a42cf

Refactoring code [1325]

89fe568

Merge branch 'main' into multi-charts

72e6603

kartik-579 reviewed Mar 22, 2022

View reviewed changes

snehith57624 added 4 commits March 22, 2022 17:48

Refactoring code [1325]

984adac

Refactoring code [1325]

66276dd

Hiding user message in error for security reasons[1325]

d738c86

Adding empty value checks for chart values [1325]

15a11a2

snehith57624 requested a review from pghildiyal March 26, 2022 13:54

pghildiyal reviewed Mar 28, 2022

View reviewed changes

Updating as per PR comments[1325]

d296cc7

snehith57624 changed the title ~~Multi charts~~ feat: Multi charts Mar 29, 2022

Updating as per PR comments[1325]

6a481e4

pghildiyal approved these changes Mar 29, 2022

View reviewed changes

pghildiyal merged commit c0e913c into main Mar 29, 2022

pghildiyal deleted the multi-charts branch March 29, 2022 05:30


		dir := impl.chartTemplateService.GetDir()

		RandomChartWorkingDir := filepath.Join(RefChartDir, Location)


		import "github.com/gorilla/mux"

		type DeploymentRouter interface {

feat: Multi charts #1013

feat: Multi charts #1013

Uh oh!

Conversation

aviralsrivastava01 commented Dec 20, 2021 • edited by pghildiyal Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of change

How Has This Been Tested?

Checklist:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pghildiyal Mar 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

snehith57624 Mar 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pghildiyal Mar 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

aviralsrivastava01 commented Dec 20, 2021 •

edited by pghildiyal

Loading

pghildiyal Mar 21, 2022 •

edited

Loading

snehith57624 Mar 22, 2022 •

edited

Loading

pghildiyal Mar 22, 2022 •

edited

Loading