bug fix for deploy target config & updated test #83
Conversation
| grouped_configs[key].append(config) | ||
|
|
||
| #Identify duplicates and mark older ones for deletion | ||
| for key, configs in grouped_configs.items(): |
There was a problem hiding this comment.
Hey @rashed-k can you explain the need to do this? In the above iteration you know if you've added a deploy config based on the constructed key to the seen config list so you could mark them for delete at that time without this extra logic.
|
|
||
| grouped_configs = {} | ||
| for config in existing_configs: | ||
| key = (config['branches'], config['pullrequests'], str(config['deployTarget']['id']), str(config['weight'])) |
There was a problem hiding this comment.
Is config['weight'] a good field to create a unique key on if this is to fix duplicate configuration items? weight is generally used to control ordering and it is common practice to use 0 when no weight is given.
For example:
configs = [{
"id": 3524,
"branches": "^master$",
"pullrequests": "false",
"deployTarget": {
"created": "2021-10-06 05:05:44",
"id": 6
},
"weight": 1
},
{
"id": 3525,
"branches": "^master$",
"pullrequests": "false",
"deployTarget": {
"created": "2021-10-06 05:05:44",
"id": 6
},
"weight": 2
}
]
The key would look something like:
Created key: ('^master$', 'false', '6', '1')
Created key: ('^master$', 'false', '6', '2')
For each item and it would skip removing the duplicate entry for ^master$
|
|
||
| if not found or not uptodate: | ||
| key = (desired['branches'], desired['pullrequests'], str(desired['deployTarget']), str(desired['weight'])) | ||
| if key not in grouped_configs: |
There was a problem hiding this comment.
As above, I think we could simplify this logic loop — you can modify config when you see it first and add a exists property rather than duplicating the key logic which will make this simpler to maintain going forward.
As the previous code did not remove old duplicate target config which had same
branch,pull-request,deploytarget IDandweight(As it assumed that the desired and existing matched according to the logic and did not perform the task). I added grouped_configs to use in the function for marking addition and deletion of deploy target config.