feat: docker-compose artifact delta generation #53
Conversation
|
Merging these commits will result in the following changelog entries: Changelogsapp-update-module (feat/docker-compose-delta)New changes in app-update-module since master: Features
|
GoethalsRobbe
force-pushed
the
feat/docker-compose-delta
branch
from
bd56599 to
ac28b09
Compare
|
As stated in your comment on my previous PR, these changes should allow to generate the delta from different manifests directories. Please review and comment with your thoughts! Best regards P.S. CI/CD testing stages succeeded, however, I think a test should be added for this scenario too? |
Changelog: Title Ticket: None Signed-off-by: Robbe Goethals <[email protected]>
GoethalsRobbe
force-pushed
the
feat/docker-compose-delta
branch
from
ac28b09 to
78f2e62
Compare
|
I was able to do some of the first testing using a CI/CD pipeline. |
hey @GoethalsRobbe I am sorry I was unable to respond this past week. is this with the current master? this bug was fixed. |
great to hear from you again, I will need sometime to go through this, please give me until Wed end of day. |
Sure, no need to hurry! |
No problem at all! |
I have not found the time yet to search for the specific commit. Do you happen to know it by hard? EDIT 29/04/2024 16u28: However, the installation of the artifact now fails with another error. I'll have look tomorrow on what is going on. The delta generation seems to be ok. |
|
@merlin-northern I did not find the time yet to investigate the issue any further. Would you have any guesses? |
yes I do. thank you for being patient. so, during past days I tested your patch and I found a bit of problems, unrelated to you code, which works flawlessly. the error you see is a problem with delta generation. please use tomorrow I will give your code a review, so far I was a bit carried away with the hands-on testing. you see I needed this feature :) |
There was a problem hiding this comment.
really nice job @GoethalsRobbe . thank you. the only question is: are you willing to write a test for it, or would you prefer me to write one?
| @@ -34,7 +34,7 @@ Usage: $0 [options] [-- [options-for-mender-artifact] ] | |||
| --output-path - Path to output artifact file. Default: reboot-artifact.mender | |||
| --image [current-url,]new-url - Path to output artifact file. Default: reboot-artifact.mender | |||
| --deep-delta - Calculate delta by parsing the image | |||
| --manifests-dir - Directory containing orchestrator-specific manifests describing the deployment | |||
| --manifests-dir [current-dir,]new-dir - Directory containing orchestrator-specific manifests describing the deployment | |||
There was a problem hiding this comment.
we should fix the description a bit, explaining what will happen when two paths are given.
| manifests_current_dir=$( echo "$2" | cut -f1 -d,) | ||
| manifests_new_dir=$( echo "$2" | cut -f2 -d,) |
There was a problem hiding this comment.
do we need those spaces?
| manifests_current_dir=$( echo "$2" | cut -f1 -d,) | |
| manifests_new_dir=$( echo "$2" | cut -f2 -d,) | |
| manifests_current_dir=$(echo "$2" | cut -f1 -d,) | |
| manifests_new_dir=$(echo "$2" | cut -f2 -d,) |
| for i in "${!current_images[@]}"; do | ||
| for j in "${!new_images[@]}"; do | ||
| # if it's a full match, don't include because it was already included in previous docker-compose | ||
| if [ "${current_images[i]}" == "${new_images[j]}" ]; then | ||
| unset 'current_images[i]' | ||
| unset 'new_images[j]' | ||
| continue 2 | ||
| # if it's a match up until the tag, include for update | ||
| elif [ "${current_images[i]%%:*}" == "${new_images[j]%%:*}" ]; then | ||
| images+=("${current_images[i]}") | ||
| images+=("${new_images[j]}") | ||
| unset 'current_images[i]' | ||
| unset 'new_images[j]' | ||
| continue 2 | ||
| fi | ||
| done | ||
| # if it isn't a match, don't include because it's removed from docker-compose | ||
| unset 'current_images[i]' | ||
| done |
There was a problem hiding this comment.
nice. have you considered using associative arrays (with declare -A array) perhaps it would save you some trouble here? if you decide to keep it this way, I would move the ifs to [[ instead single bracket.
|
@GoethalsRobbe I consider this a very worthy contribution. if you are busy and do not have the time, please let me know if we should carry it over. |
I must have missed your comments on 14/10/2024. I will not be able to make time for this PR I'm afraid. In case anyone else could have a look at this, it could hopefully get merged. |
Changelog: Title
Ticket: None