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

Prevent pihole -up errors when local repository is dirty #4798

Closed
wants to merge 1 commit into from

Conversation

rdwebdesign
Copy link
Member

Thank you for your contribution to the Pi-hole Community!

Please read the comments below to help us consider your Pull Request.

We are all volunteers and completing the process outlined will help us review your commits quicker.

Please make sure you

  1. Base your code and PRs against the repositories developmental branch.
  2. Sign Off all commits as we enforce the DCO for all contributions

What does this PR aim to accomplish?:

When users make changes to their pi-hole code directly on master branch, the update fails because git can't replace the code.
Example: pi-hole/web#2255

This fixes this error.

How does this PR accomplish the above?:

Inverting the execution order (running git reset --hard before git pull).

What documentation changes (if any) are needed to support this PR?:

none


By submitting this pull request, I confirm the following:

  1. I have read and understood the contributors guide, as well as this entire template. I understand which branch to base my commits and Pull Requests against.
  2. I have commented my proposed changes within the code and I have tested my changes.
  3. I am willing to help maintain this change if there are issues with it later.
  4. It is compatible with the EUPL 1.2 license
  5. I have squashed any insignificant commits. (git rebase)

  • I have read the above and my PR is ready for review.

@yubiuser
Copy link
Member

yubiuser commented Jul 8, 2022

I think this is a good point to be a bit more verbose. Let users know we are stashing their changes and resetting and pulling.

its0x08
its0x08 previously approved these changes Jul 10, 2022
@yubiuser
Copy link
Member

I tried to test this but testing failed, because I could not create a condition where there original code failed to update the repo. I made changes to the local repo (and even tried to committed them) and used the following script to trigger an "update".

#!/usr/bin/env bash
COL_NC='\e[0m' # No Color
COL_LIGHT_GREEN='\e[1;32m'
COL_LIGHT_RED='\e[1;31m'
TICK="[${COL_LIGHT_GREEN}✓${COL_NC}]"
CROSS="[${COL_LIGHT_RED}✗${COL_NC}]"
INFO="[i]"
# shellcheck disable=SC2034
DONE="${COL_LIGHT_GREEN} done!${COL_NC}"
OVER="\\r\\033[K"
    

ADMIN_INTERFACE_DIR="/var/www/html/admin"
ADMIN_INTERFACE_GIT_URL="https://github.com/pi-hole/AdminLTE.git"


update_repo() {
    # Use named, local variables
    # As you can see, these are the same variable names used in the last function,
    # but since they are local, their scope does not go beyond this function
    # This helps prevent the wrong value from being assigned if you were to set the variable as a GLOBAL one
    local directory="${1}"
    local curBranch

    # A variable to store the message we want to display;
    # Again, it's useful to store these in variables in case we need to reuse or change the message;
    # we only need to make one change here
    local str="Update repo in ${1}"
    # Move into the directory that was passed as an argument
    pushd "${directory}" &> /dev/null || return 1
    # Let the user know what's happening
    printf "  %b %s..." "${INFO}" "${str}"
    # Stash any local commits as they conflict with our working code
    git stash --all --quiet &> /dev/null || true # Okay for stash failure
    git clean --quiet --force -d || true # Okay for already clean directory
    # Pull the latest commits
    git pull --no-rebase --quiet &> /dev/null || return $?
    # Check current branch. If it is master, then reset to the latest available tag.
    # In case extra commits have been added after tagging/release (i.e in case of metadata updates/README.MD tweaks)
    curBranch=$(git rev-parse --abbrev-ref HEAD)
    if [[ "${curBranch}" == "master" ]]; then
        git reset --hard "$(git describe --abbrev=0 --tags)" || return $?
    fi
    # Show a completion message
    printf "%b  %b %s\\n" "${OVER}" "${TICK}" "${str}"
    # Data in the repositories is public anyway so we can make it readable by everyone (+r to keep executable permission if already set by git)
    chmod -R a+rX "${directory}"
    # Move back into the original directory
    popd &> /dev/null || return 1
    return 0
}


update_repo "${ADMIN_INTERFACE_DIR}" || { printf "\\n  %b: Could not update local repository. Contact support.%b\\n" "${COL_LIGHT_RED}" "${COL_NC}"; exit 1; }

@rdwebdesign
Copy link
Member Author

I tried to test this but testing failed, because I could not create a condition where there original code failed to update the repo.

I have the same problem to test.
I can't replicate the conditions where my local repo have correct "state" to execute the test.

@rdwebdesign
Copy link
Member Author

Setting as draft.
This will be tested after the next release.

@rdwebdesign rdwebdesign marked this pull request as draft August 31, 2022 21:30
@rdwebdesign rdwebdesign marked this pull request as ready for review September 3, 2022 03:59
@rdwebdesign rdwebdesign marked this pull request as draft September 3, 2022 04:34
also adding line breaks for readability

Signed-off-by: RD WebDesign <github@rdwebdesign.com.br>
@rdwebdesign
Copy link
Member Author

I know sometimes an error happens (see issue 2255), but I'm not able to reproduce the exact conditions to trigger the error and test this PR.

@rdwebdesign
Copy link
Member Author

Closing this PR.

I don't remember this error happening recently and there was no reliable way to test the code.

@rdwebdesign rdwebdesign deleted the reset_before_pull branch January 24, 2023 20:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Tracking] Update repo should reset before pulling to prevent errors on unclean repos
7 participants