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.

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

Mismatched or dirty local k/release copy does not block krel gcbmgr runs #1278

Closed
justaugustus opened this issue May 11, 2020 · 3 comments · Fixed by #1284
Closed

Mismatched or dirty local k/release copy does not block krel gcbmgr runs #1278

justaugustus opened this issue May 11, 2020 · 3 comments · Fixed by #1284
Assignees
Labels
area/release-eng Issues or PRs related to the Release Engineering subproject kind/bug Categorizes issue or PR as related to a bug. needs-priority sig/release Categorizes an issue or PR as relevant to SIG Release.
Milestone

Comments

@justaugustus
Copy link
Member

What happened:

The repo state check in #1254 does not appear to block gcbmgr runs with a dirty or out of sync local copy of k/release.

/assign @saschagrunert

@justaugustus justaugustus added kind/bug Categorizes issue or PR as related to a bug. sig/release Categorizes an issue or PR as relevant to SIG Release. area/release-eng Issues or PRs related to the Release Engineering subproject labels May 11, 2020
@justaugustus justaugustus changed the title Mismatched or dirty k/release does not block r Mismatched or dirty k/release does not block krel gcbmgr runs May 11, 2020
@justaugustus justaugustus changed the title Mismatched or dirty k/release does not block krel gcbmgr runs Mismatched or dirty local k/release copy does not block krel gcbmgr runs May 11, 2020
@justaugustus justaugustus added this to the v1.19 milestone May 11, 2020
@saschagrunert
Copy link
Member

Yes, my template for the implementation was this function, which do not check those cases as well:

release/lib/gitlib.sh

Lines 296 to 328 in 61c2711

gitlib::repo_state () {
local expectedOrg="${TOOL_ORG:-kubernetes}"
local expectedRepo="${TOOL_REPO:-release}"
local expectedRemote="[:/]${expectedOrg}/${expectedRepo}"
local expectedBranch="${TOOL_BRANCH:-master}"
local branch
branch=$(gitlib::current_branch "$TOOL_ROOT") || return 1
if [ "${expectedBranch}" != "$branch" ]
then
logecho "$FAILED checked out branch $branch is not the same as $expectedBranch"
return 1
fi
local remotePattern="${expectedRemote}(.git)* \(fetch\)$"
local remote=$( git -C "$TOOL_ROOT" remote -v | grep -E "$remotePattern" -m 1 | cut -f1 )
local commit=$(git -C "$TOOL_ROOT" \
ls-remote --heads "$remote" refs/heads/master | cut -f1)
local output=$(git -C "$TOOL_ROOT" branch --contains "$commit" "$branch" 2>/dev/null)
logecho -n "Checking $TOOL_ROOT state: "
if [[ -n "$output" ]]; then
logecho $OK
else
logecho "$FAILED"
logecho
logecho "$TOOL_ROOT is not up to date."
logecho "$ git pull"
return 1
fi
}

So we can add two possible checks:

  • Error if the repository is dirty
  • Error if the current commit does not match exactly the latest remote commit

In case of the second error path the branch --contains check could be replaces with a basic SHA comparison. WDYT? Should we add both?

@justaugustus
Copy link
Member Author

@saschagrunert -- Let's aim to check for both. The shell-based gcbmgr did check for the latter.

@saschagrunert
Copy link
Member

Fix is incoming in #1284

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/release-eng Issues or PRs related to the Release Engineering subproject kind/bug Categorizes issue or PR as related to a bug. needs-priority sig/release Categorizes an issue or PR as relevant to SIG Release.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants