-
Notifications
You must be signed in to change notification settings - Fork 237
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
WIP: Add script to update dockerfiles on a PR merge #466
base: master
Are you sure you want to change the base?
WIP: Add script to update dockerfiles on a PR merge #466
Conversation
update_dockerfiles_on_merge.sh
Outdated
git branch -D ${BRANCH_NAME} | ||
git checkout -b ${BRANCH_NAME} | ||
fi | ||
source update_all.sh |
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.
Please add a variable to only update release build Dockerfiles (and not nightly ones)
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.
We can now pass the first arg as releases or nightly, if nothing is passed it updates both releases and nightly
update_dockerfiles_on_merge.sh
Outdated
git checkout -b ${BRANCH_NAME} | ||
fi | ||
source update_all.sh | ||
git add 8/ 9/ 10/ 11/ 12/ 13/ 14/ 15/ |
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 needs to happen based on supported_versions
available in common_functions.sh
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.
made changes accordingly
update_dockerfiles_on_merge.sh
Outdated
echo -e "${BRED}CAUTION${NOCOLOR} : Please run this script after saving your work (commiting) as ${RED}it may mess up your changes${NOCOLOR}." | ||
echo "" | ||
echo "" | ||
read -r -p "Do you want to proceed ? [y/N] " USER_RESPONSE |
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.
We need a non-interactive mode so that we can call this from a jenkins job based on a trigger
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.
removed the non-interactive mode
2ce9e98
to
d2d981e
Compare
update_multiarch.sh
Outdated
@@ -43,7 +45,11 @@ do | |||
btypes=$(parse_vm_entry "${vm}" "${version}" "${package}" "${os}" "Type:") | |||
dir=$(parse_vm_entry "${vm}" "${version}" "${package}" "${os}" "Directory:") | |||
|
|||
for build in ${builds} | |||
if [ ! -z "${build_arg}" ]; 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.
Looks like you have spaces instead of tabs here
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.
changed spaces to tabs
update_all.sh
Outdated
@@ -17,6 +17,11 @@ set -o pipefail | |||
# shellcheck source=common_functions.sh | |||
source ./common_functions.sh | |||
|
|||
if [ -n "$1" ]; then | |||
build_arg="$1" |
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.
spaces here ... should be tabs
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.
changed spaces to tabs
@@ -0,0 +1,125 @@ | |||
#!/bin/bash |
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.
Maybe you need to fix your editor ... this entire file uses spaces.
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.
updated the file to have tabs instead of spaces
update_dockerfiles_on_merge.sh
Outdated
fi | ||
source update_all.sh releases | ||
git add ${supported_versions} | ||
git commit -m "Updating dockerfiles via update_dockerfiles_on_merge.sh script" -s |
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.
The commit message should have a date and timestamp along with the PR number, something like
Timestamp: Auto-updating dockerfiles for PR #"${PR_NUMBER}"
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.
Added custom timestamp : git commit -m "$(date '+%d-%m-%Y %H:%M:%S') : Auto-updating dockerfiles for PR #${PR_NUM}" -s
Signed-off-by: bharathappali <[email protected]>
d2d981e
to
99967b8
Compare
@bharathappali I think I've got a fully automated solution here: #558 |
As part of dockerfiles generation automation process, I would like to add this script to the repo to semi automate the process of updating dockerfiles after a PR with dockerfile changes gets merged.
Why its a semi automation ?
User who runs this script needs to manually raise a PR for updating the dockerfiles. The generation of dockerfiles is only automated.
What would be a better solution ?
triggering a automatic dockerfiles update after a PR merge via github actions or github webhooks will be an ideal solution. Till we explore a possible fix, this script can be handy
@dinogun Can i have your views on it ?
Signed-off-by: bharathappali [email protected]