Skip to content
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.

Sign up for GitHub

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

Jump to bottom

ENT-11970: Moved masterfiles-stage checks for git in sync so that they can work properly #5577

Closed
wants to merge 2 commits into from
Closed

ENT-11970: Moved masterfiles-stage checks for git in sync so that they can work properly #5577

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d280bd3
Moved masterfiles-stage checks for git in sync so that they can work …
craigcomstock Jul 10, 2024
6497791
Added logging of fetch --dry-run in masterfiles stage scripts for ref…
craigcomstock Jul 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 27 additions & 18 deletions contrib/masterfiles-stage/common.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ git_setup_local_mirrored_repo() {
# /opt/cfengine/git_mygitserver.net_joeblow_my_policy_repo.git

if [ -d "${local_mirrored_repo}" ] ; then
git --git-dir="${local_mirrored_repo}" fetch --dry-run # to provide information in logs about from and to commits
vpodzime marked this conversation as resolved.
Show resolved Hide resolved
git --git-dir="${local_mirrored_repo}" fetch && return 0

# If execution arrives here, the local_mirrored_repo exists but is messed up somehow
Expand Down Expand Up @@ -112,9 +113,6 @@ git_deploy_refspec() {
mkdir -p "${temp_stage}/.git"
cp "${local_mirrored_repo}/HEAD" "${temp_stage}/.git/"

if git_check_is_in_sync "${local_mirrored_repo}" "${temp_stage}" "$2"; then
return 0
fi
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why should this not work? The mirror repo is already cloned/fetched/checked out, right? It's here on purpose to ensure early return without overwriting all of /var/cfengine/masterfiles with the same revision.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem here I think is that we have copied HEAD into temp_stage so it is always the same.

As-is I never got updates. There may be another cause but if you check my notes in ENT-11970 you might see some details that shed some light on the situation.

I imagine it worked for you while testing so not sure what is going on. Maybe grab a nightly build and try it out there. That's what I was doing.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I imagine it worked for you while testing so not sure what is going on.

Unfortunately, I didn't test this very much. I was planning to test the whole set of changing how and where from masterfiles-stage.sh is run, but then I got busy with other things before finishing the set.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem here I think is that we have copied HEAD into temp_stage so it is always the same.

Oh, I see it now! But IMHO the error here is to compare with ${temp_stage}, we should compare with $MASTERDIR (or whatever it's called) and keep doing it here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup, looks like that's enough: #5579 (and https://github.com/cfengine/nova/pull/2284)


########################## 3. SET PERMISSIONS ON POLICY SET
chown -R root:root "${temp_stage}" || error_exit "Unable to chown '${temp_stage}'"
Expand Down Expand Up @@ -238,9 +236,6 @@ git_cfbs_deploy_refspec() {
mkdir -p "${temp_stage}/out/masterfiles/.git"
cp "${local_mirrored_repo}/HEAD" "${temp_stage}/out/masterfiles/.git/"

if git_check_is_in_sync "${local_mirrored_repo}" "${temp_stage}/out/masterfiles" "$2"; then
return 0
fi

########################## 3. SET PERMISSIONS ON POLICY SET
chown -R root:root "${temp_stage}" || error_exit "Unable to chown '${temp_stage}'"
Expand Down Expand Up @@ -385,15 +380,22 @@ git_masterstage() {
# Depends on $GIT_URL, $ROOT, $MASTERDIR, $GIT_REFSPEC
check_git_installed
git_setup_local_mirrored_repo "$( dirname "$ROOT" )"
if [ "x$1" = "xtrue" ]; then
if git_check_is_in_sync "$( dirname "$ROOT" )" "$MASTERDIR" "$GIT_REFSPEC"; then
return 1 # in sync => nothing to do
git_check_is_in_sync "$local_mirrored_repo" "$MASTERDIR" "$GIT_REFSPEC"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be nice to mention in a comment here that we are abusing the previous function setting a global variable so we know local_mirrored_repo

local in_sync=$?
if [ "$1" = "true" ]; then
if [ "$in_sync" = "0" ]; then
return 1 # in sync => nothing to do
else
return 0 # not in sync => update available
fi
else
if [ "$in_sync" != "0" ]; then
git_deploy_refspec "$MASTERDIR" "${GIT_REFSPEC}"
echo "Successfully deployed '${GIT_REFSPEC}' from '${GIT_URL}' to '${MASTERDIR}' on $(date)"
else
return 0 # not in sync => update available
echo "'${GIT_REFSPEC}' from '${GIT_URL}' has no changes. Will not update."
fi
fi
git_deploy_refspec "$MASTERDIR" "${GIT_REFSPEC}"
echo "Successfully deployed '${GIT_REFSPEC}' from '${GIT_URL}' to '${MASTERDIR}' on $(date)"
}

git_cfbs_masterstage() {
Expand All @@ -402,15 +404,22 @@ git_cfbs_masterstage() {
check_git_installed
check_cfbs_installed
git_setup_local_mirrored_repo "$( dirname "$ROOT" )"
if [ "x$1" = "xtrue" ]; then
if git_check_is_in_sync "$( dirname "$ROOT" )" "$MASTERDIR" "$GIT_REFSPEC"; then
return 1 # in sync => nothing to do
git_check_is_in_sync "$local_mirrored_repo" "$MASTERDIR" "$GIT_REFSPEC"
local in_sync=$?
if [ "$1" = "true" ]; then
if [ "$in_sync" = "0" ]; then
return 1 # in sync => nothing to do
else
return 0 # not in sync => update available
fi
else
if [ "$in_sync" != "0" ]; then
git_cfbs_deploy_refspec "$MASTERDIR" "${GIT_REFSPEC}"
echo "Successfully built and deployed '${GIT_REFSPEC}' from '${GIT_URL}' to '${MASTERDIR}' on $(date) with cfbs"
else
return 0 # not in sync => update available
echo "'${GIT_REFSPEC}' from '${GIT_URL}' has no changes. Will not update."
fi
fi
git_cfbs_deploy_refspec "$MASTERDIR" "${GIT_REFSPEC}"
echo "Successfully built and deployed '${GIT_REFSPEC}' from '${GIT_URL}' to '${MASTERDIR}' on $(date) with cfbs"
}

svn_branch() {
Expand Down
Loading