-
Notifications
You must be signed in to change notification settings - Fork 18
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
#7224 Pull images parallel during deployment #1129
base: develop
Are you sure you want to change the base?
Conversation
infrastructure/deployment/deploy.sh
Outdated
echo "-----" | ||
cat docker_images.txt | ||
echo "-----" | ||
#configured_ssh "cd /opt/opencrvs && docker pull $tag" |
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.
Let's clean up commits and debug messages from PRs
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.
sure will do the changes.
infrastructure/deployment/deploy.sh
Outdated
echo "Server failed to download $tag. Retrying..." | ||
sleep 5 | ||
done | ||
echo -e "$tag \n" >> docker_images.txt |
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.
Is there a benefit for the extra step of first writing the IMAGE_TAGS_TO_DOWNLOAD[@]
list to a file? Originally this loop's purpose was downloading the images, so we could keep it at that.
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.
This file contains all the images list which are present on the machine
infrastructure/deployment/deploy.sh
Outdated
cat docker_images.txt | ||
echo "-----" | ||
#configured_ssh "cd /opt/opencrvs && docker pull $tag" | ||
pull_images_from_file "docker_images.txt" |
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.
Creating a file shouldn't be necessary for passing the tags forward, right? Use function arguments instead
infrastructure/deployment/deploy.sh
Outdated
# Check if the file exists | ||
if [[ ! -f "$file" ]]; then | ||
echo "File not found: $file" | ||
exit 1 | ||
fi |
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 you generate code with ChatGPT (allowed, of course) please clean up formatting to match the rest of the file. Maybe there's a linter for shell scripts we could start using? The generated comments are usually pretty self-explanatory asa well so I tend to clean those away from the code just to reduce noise.
Besides that, it's good to properly read the generated code through. For instance in this code I highlighted, we are checking a file exists that we created a few steps backwards.
infrastructure/deployment/deploy.sh
Outdated
continue | ||
fi | ||
echo "Pulling $image..." | ||
if docker pull "$image"; then |
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.
On which machine does this image get pulled to? I looked at the images on the farajaland-dev machine right after deployment
riku@farajaland-dev:~$ docker images | grep fara
opencrvs/ocrvs-farajaland <none> 5d511c748fe0 About an hour ago 1.54GB
opencrvs/ocrvs-farajaland 3534c7a 4e85e3bed115 About an hour ago 1.54GB
A moment later (docker swarm downloads images automatically later on in the process if images are not present). First one is the one that got deployed.
riku@farajaland-dev:~$ docker images | grep fara
opencrvs/ocrvs-farajaland <none> af7798ce0120 23 minutes ago 1.54GB
opencrvs/ocrvs-farajaland <none> 5d511c748fe0 About an hour ago 1.54GB
opencrvs/ocrvs-farajaland 3534c7a 4e85e3bed115 About an hour ago 1.54GB
sure will do the changes as suggested. |
reopening the PR |
No description provided.