-
Notifications
You must be signed in to change notification settings - Fork 185
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
ENT-11970: Moved masterfiles-stage checks for git in sync so that they can work properly #5577
Conversation
…properly Ticket: ENT-11970 Changelog: none
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" |
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.
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
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.
I don't quite understand what's going on here. Please explain things in the commit messages.
@@ -112,9 +112,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 |
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.
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.
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 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.
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.
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.
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 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.
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.
yup, looks like that's enough: #5579 (and https://github.com/cfengine/nova/pull/2284)
Other fixes have been made. No need for this PR any more. :) |
Ticket: ENT-11970
Changelog: none