Next download feature 2.0 #421
Conversation
| dependenciesPerEnvironment := make(map[string][]string) | ||
| return dependenciesPerEnvironment | ||
| } | ||
| func solveDependencies(projectId string, configs map[string][]configv2.Config) map[string][]configv2.Config { |
There was a problem hiding this comment.
This might be a good case for a parallel foreach implementation https://stackoverflow.com/questions/24238820/parallel-for-loop
| for _, api := range listApis { | ||
| creator := dynatraceparser.NewDynatraceParser() | ||
| configs, errs := createConfigforTargetApi(env, creator, client, api, projectId) | ||
| //apiconfig, err := createConfigFromApiV2(env, api, projectId, token, client) |
|
|
||
| package dynatraceparser | ||
|
|
||
| //filterConfiguration evaluates multiple filters to check if downloaded configuration is valid |
There was a problem hiding this comment.
Comment should have the following format 'NotSupportedConfiguration ...' (with an optional leading article)
| if len(errors) > 0 { | ||
| return nil, nil, errors | ||
| } | ||
| projectDefinitions[projectDefinition.Name] = projectDefinition |
There was a problem hiding this comment.
I may be missing something, but don't you build a map (and list) here that always just contains one element?
There was a problem hiding this comment.
yes, I'm reusing the entities and methods from convert command. That one takes multiple projects and multiple arrays in general. I could create single implementations of the configs but I would be duplicating code and I didn't want to modify much of the convert command code including methods and entities.
| } | ||
|
|
||
| //generateManifest builds a manifest with a single environment and project | ||
| func generateManifest(fs afero.Fs, workdir string, |
There was a problem hiding this comment.
seems odd to me that you directly write to disk and return a manifest that you never actually use (as far as I can see)
might be worth considering to actually split "generate" and "write" manifest - which would also allow you to test the generation on its own
| //returns the list of API filter if the download specific flag is used, otherwise returns all the API's | ||
| func getAPIList(downloadSpecificAPI string) (filterAPIList map[string]api.Api, err error) { | ||
| availableApis := api.NewApis() | ||
| noFilterAPIListProvided := strings.TrimSpace(downloadSpecificAPI) == "" |
There was a problem hiding this comment.
nitpick: you trimSpace of the input here, then if there's content trim each of the split APIs again.
You could trim the complete string once, then re-use that for splitting.
| } | ||
| } | ||
| if isErr { | ||
| return nil, fmt.Errorf("There were some errors in the API list provided") |
There was a problem hiding this comment.
not sure if we want to fail on this - with the general concept of "fail only if you can't proceed at all" (as we e.g. follow on upload) we could maybe just download the APIs we know and at the end tell the user "hey, downloaded A,B,C but D is not an API we know"
@didiladi , @tatomir146 what do you think?
There was a problem hiding this comment.
For the upload functionality we default to fail but can overrule this with -c parameter. The option would be to use this flag also for download functionality, or as I would prefer, to do it as @UnseenWizzard described.
| for _, configType := range configs { | ||
| for _, config := range configType { | ||
| content := config.Template.Content() | ||
| for key, val := range tempConfig { |
There was a problem hiding this comment.
please use something more explicit like id, coordinate
There was a problem hiding this comment.
TempConfig does not check for empty ID's, which causes major issues when replacing further on. Example from a real customer environment:
That turns your app detection rule into this:
And the memory consumption into this:
|
|
||
| package downloader | ||
|
|
||
| // func TestCreateConfigsFromAPI(t *testing.T) { |
There was a problem hiding this comment.
please comment in, or delete
| Value: name, | ||
| } | ||
|
|
||
| // if len(errors) > 0 { |
There was a problem hiding this comment.
please delete commented out code
| func getTemplate(client rest.DynatraceClient, creator dynatraceparser.DynatraceParser, api api.Api, | ||
| val api.Value, path string) (template.Template, map[string]parameter.Parameter, string, bool, error) { | ||
|
|
||
| file, dynatraceId, fullpath, name, filter, err := creator.GetConfig(client, api, val, path) |
There was a problem hiding this comment.
I find the way the 'filter' boolean to filter things out is passed through methods pretty hard to grasp/follow
Haven't grasped the handling fully, so no clue if that could actually be improved, but for readability it would be preferably to filter on the outside, before calling things, rather than returning "filter" from the end of the callchain
| newContent := strings.ReplaceAll(config.Template.Content(), dependencyCoor.DynatraceId, dependencyCoor.Config) | ||
| idProperty := "id" //since all depends on id to relative configs | ||
| ref := refParam.New(dependencyCoor.Project, dependencyCoor.Api, dependencyCoor.Config, idProperty) | ||
| config.Template.UpdateContent(newContent) |
There was a problem hiding this comment.
would personally rather create a new Template with the newContent here, than extending it with an UpdateContent
generally it's not something that will be modified, so I don't think we should make it possible just for this use
|
|
|
this is good and very important work in this pull request the download feature and command options should maintain consistency with the deploy command options The download feature should use the same manifest file as the deployment file to reference environments to download configurations from and get rid of the --environment flag |
Co-authored-by: tatomir146 <[email protected]>
Co-authored-by: tatomir146 <[email protected]>
Co-authored-by: tatomir146 <[email protected]>
Co-authored-by: tatomir146 <[email protected]>
Co-authored-by: tatomir146 <[email protected]>
Co-authored-by: tatomir146 <[email protected]>
|
feature was integrated into v2 dev branch |
Created the PR for Monaco download 2.0. Still implementing the unit and integration test but everyone can grab the code and start testing/ reviewing it. I have reuse most of the entities from the convert command.
Pending tasks: