Allow to checkout tags via `pihole checkout` #5259

yubiuser · 2023-04-21T20:55:30Z

What does this PR aim to accomplish?:

Allow to checkout specific tags via pihole checkout for core and web. So far only branches were allowed.

P.S. Checking out tags for FTLwas possible already (as we don't download via git repo but a binary directly from our own server).

How does this PR accomplish the above?:

Fetch with --tags added when getting upstream references.
When performing a "checkout" we check the upstream repo if the requested ref is a branch or a tag and depending on that we create a new branch with or without --tracking set (which is set by default when using checkout without -b/B)
"Guard" all git pull executions by first checking if the current branch is a branch or tag upstreams and skipping the pull in the latter case (does not make any sense to pull in case we checked out a tag)
Moves all relevant functions from basic-install.sh to piholeCheckout.sh as they have been used only there and nowhere else.

By submitting this pull request, I confirm the following:

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.
I have commented my proposed changes within the code and I have tested my changes.
I am willing to help maintain this change if there are issues with it later.
It is compatible with the EUPL 1.2 license
I have squashed any insignificant commits. (git rebase)
I have checked that another pull request for this purpose does not exist.
I have considered, and confirmed that this submission will be valuable to others.
I accept that this submission may not be used, and the pull request closed at the will of the maintainer.
I give this submission freely, and claim no ownership to its content.

I have read the above and my PR is ready for review. Check this box to confirm

rdwebdesign · 2023-04-21T21:15:11Z

I tried the branch (I know it's still a draft).

This is the current output:

root@pihole:/# pihole checkout web v5.15          
  Please note that changing branches or tags severely alters your Pi-hole subsystems
  Features that work on the master branch, may not on a development branch
  This feature is NOT supported unless a Pi-hole developer explicitly asks!
  Have you read and understood this? [y/N] y

  [✓] Fetching branches/tags from https://github.com/pi-hole/AdminLTE.git
  [i] 127 branches/tags available for Web Admin

  [✓] Switching to branch/tag: 'v5.15' from 'refs/heads/devel'
You are not currently on a branch.
Please specify which branch you want to merge with.
See git-pull(1) for details.

    git pull <remote> <branch>

The tag was correctly used:

root@pihole:/var/www/html/admin# git status
HEAD detached at v5.15
nothing to commit, working tree clean

But pihole -v still shows the branch devel and the original commit:

root@pihole:/var/www/html/admin# pihole -v
  Pi-hole version is checkout/tags v5.15.5-81-g6d2ceca6 (Latest: v5.16.2)
  AdminLTE version is devel v5.19-33-gefddf9c7 (Latest: v5.19)
  FTL version is development vDev-b0bf9c0 (Latest: v5.22)

EDIT:
After executing pihole updatechecker the tag is shown correctly:

root@pihole:/var/www/html/admin# pihole updatechecker
root@pihole:/var/www/html/admin# pihole -v                
  Pi-hole version is checkout/tags v5.15.5-81-g6d2ceca6 (Latest: v5.16.2)
  AdminLTE version is HEAD v5.15 (Latest: v5.19)
  FTL version is development vDev-b0bf9c0 (Latest: v5.22)

advanced/Scripts/piholeCheckout.sh

Signed-off-by: Christian König <[email protected]>

yubiuser · 2023-04-23T12:13:28Z

This PR needed more changes than I thought. I squashed and forced push, now there are two commits: the first includes all necessary changes (for easier review) and the second moves the relevant functions from basic-install.sh to piholeCheckout.sh.
I'll adjust the PR message to explain all changes. The PR should be ready now for testing.

Signed-off-by: Christian König <[email protected]>

DL6ER

Really very minor criticism at this point (only two typos). I will have to test this in on a new install with a shallow cloned repository to be meaningful

advanced/Scripts/update.sh

automated install/basic-install.sh

Co-authored-by: DL6ER <[email protected]> Signed-off-by: yubiuser <[email protected]>

DL6ER

What I did during review was creating a fresh VM and installing latest Pi-hole on it. Then I replaced the files in ´/opt/pihole` with the versions proposed in this PR.

Then, I tried pihole checkout web v5.18:

Seems to work fine.

Then I repeated this for core (where v5.18 does not exist), and this seems to work, too:

(cropped screenshot)

This also revealed some interesting tag that does not show up on Github:

All in all this seems to be doing what it supposed to do. I don't think we'll be able to anticipate any possible user system (mis)configuration but for a plain simple-and-easy standard installation, this seems to work fine.

As changing the update.sh script has the potential to break things in a bad way, I'd liek to ask @dschaper for his opinion. The change in this script is small.

DL6ER · 2023-05-12T08:03:52Z

advanced/Scripts/update.sh

+ # check if the local branch ref is a branch or a tag on remote repo
+ if git show-ref -q --verify "refs/remotes/origin/$curBranch" 2>/dev/null; then
+ ref_is_tag=false
+ elif git show-ref -q --verify "refs/tags/$curBranch" 2>/dev/null; then
+ ref_is_tag=true
+ fi


I see no else here, what happens if neither of the two are true? ref_is_tag seems to stay random in this case (empty, likely). Can this happen? (I'm thinking about users doing local changes and committing them so the head is not pointing to anything on origin and also isn't a tag)

DL6ER · 2023-05-12T08:05:14Z

automated install/basic-install.sh

+ if git show-ref -q --verify "refs/remotes/origin/$curBranch" 2>/dev/null; then
+ ref_is_branch=true
+ elif git show-ref -q --verify "refs/tags/$curBranch" 2>/dev/null; then
+ ref_is_branch=false
+ fi


See my comment in update.sh, this is the same code.

DL6ER · 2023-05-12T08:30:20Z

Well, I hit [Comment] too soon. I did try to checkout the latest tag (coming from most recent master) but this failed:

root@ubuntu-s-4vcpu-8gb-fra1-01:/etc/.pihole# pihole checkout core v5.16.2
  Please note that changing branches or tags severely alters your Pi-hole subsystems
  Features that work on the master branch, may not on a development branch
  This feature is NOT supported unless a Pi-hole developer explicitly asks!
  Have you read and understood this? [y/N] y

  [✓] Fetching branches/tags from https://github.com/pi-hole/pi-hole.git
  [i] 122 branches/tags available for Pi-hole Core

  [✓] Switching to branch/tag: 'v5.16.2' from 'refs/heads/master'
  [i] Running installer to upgrade your installation

  [✓] Root user check

        .;;,.
        .ccccc:,.
         :cccclll:.      ..,,
          :ccccclll.   ;ooodc
           'ccll:;ll .oooodc
             .;cll.;;looo:.
                 .. ','.
                .',,,,,,'.
              .',,,,,,,,,,.
            .',,,,,,,,,,,,....
          ....''',,,,,,,'.......
        .........  ....  .........
        ..........      ..........
        ..........      ..........
        .........  ....  .........
          ........,,,,,,,'......
            ....',,,,,,,,,,,,.
               .',,,,,,,,,'.
                .',,,,,,'.
                  ..'''.

  [i] SELinux not detected
  [✓] Update local cache of available packages
  [i] Existing PHP installation detected : PHP version 8.1.2-1ubuntu2.11

  [✓] Checking apt-get for upgraded packages... 49 updates available
  [i] It is recommended to update your OS after installing the Pi-hole!

  [i] Checking for / installing Required dependencies for OS Check...
  [✓] Checking for grep
  [✓] Checking for dnsutils

  [✓] Supported OS detected
  [i] Checking for / installing Required dependencies for this install script...
  [✓] Checking for git
  [✓] Checking for iproute2
  [✓] Checking for dialog
  [✓] Checking for ca-certificates

  [i] Performing unattended setup, no dialogs will be displayed
  [✓] Check for existing repository in /etc/.pihole
  [i] Update repo in /etc/.pihole...
  : Could not update local repository. Contact support.
   Error: Unable to complete update, please contact support

root@ubuntu-s-4vcpu-8gb-fra1-01:/etc/.pihole# git status
On branch v5.16.2
nothing to commit, working tree clean

root@ubuntu-s-4vcpu-8gb-fra1-01:/etc/.pihole# git pull
There is no tracking information for the current branch.
Please specify which branch you want to merge with.
See git-pull(1) for details.

    git pull <remote> <branch>

If you wish to set tracking information for this branch you can do so with:

    git branch --set-upstream-to=origin/<branch> v5.16.2

yubiuser · 2023-05-12T08:58:41Z

The failure is expected as you only replaced the files in /opt/pihole but not the local repo in /etc/.pihole. We call the installer from the repo after checkout of core

pi-hole/advanced/Scripts/piholeCheckout.sh

Lines 282 to 283 in ff91db7

 echo -e " ${INFO} Running installer to upgrade your installation" 

 if "${PI_HOLE_FILES_DIR}/automated install/basic-install.sh" --unattended; then

The old file is missing the handling of tags

pi-hole/automated install/basic-install.sh

Lines 488 to 492 in ff91db7

 # only pull if we are on a branch not a tag 

 if [ "$ref_is_branch" = true ]; then 

 # Pull the latest commits 

 git pull --no-rebase --quiet &> /dev/null || return $? 

 fi

DL6ER · 2023-05-12T10:04:11Z

I also replaced them in the repo (and committed that to master to not be in a dirty state), but this was obviously overwritten with

  [✓] Switching to branch/tag: 'v5.16.2' from 'refs/heads/master'
  [i] Running installer to upgrade your installation

So is this a general issue here? Whenever users will want to checkout something before (the coming) v5.17 possibly including this, they will always run the installer that is not aware of tags or am I mistaken here?

yubiuser · 2023-05-12T12:34:05Z

I fear you are right. Checking out any tag before this is merged and tagged will fail on core because the handing of tags obviously was not implemented back then.
How to proceed? We could prevent checking out tags before 5.17?

DL6ER · 2023-05-13T07:11:32Z

We could prevent checking out tags before 5.17?

I'd say it depends on how much code would be required to do this. We could also simply show an additional warning when we detect that users try to checkout core + tag telling them that below v5.17 won't work and giving the option to cancel without actually doing anything [y/N] at this point

yubiuser · 2023-05-13T07:15:55Z

It would be quite some code.... I tried yesterday: we would separate getting branches and tags, convert the tags to numbers (could re-use some code from PADD) and only include tags above a certain number.

yubiuser · 2023-05-13T07:33:48Z

pi@s740:~$ pihole checkout core v5.9.1
  Please note that changing branches or tags severely alters your Pi-hole subsystems
  Features that work on the master branch, may not on a development branch
  This feature is NOT supported unless a Pi-hole developer explicitly asks!
  Have you read and understood this? [y/N] y

  [✓] Fetching branches/tags from https://github.com/pi-hole/pi-hole.git
  [i] 122 branches/tags available for Pi-hole Core

  [i] Switching to branch/tag: 'v5.9.1' from 'refs/heads/checkout/tags'

  [i] Your are trying to checkout the tag v5.9.1 on the core repo
  Please note that checking out core tags <v5.17  will break your local repo 
  Proceed anyway? [y/N]

Signed-off-by: Christian König <[email protected]>

pralor-bot · 2023-05-28T08:34:57Z

This pull request has been mentioned on Pi-hole Userspace. There might be relevant details there:

https://discourse.pi-hole.net/t/how-do-i-revert-to-a-previous-version-of-pi-hole/7168/28

rdwebdesign previously approved these changes Apr 22, 2023

View reviewed changes

advanced/Scripts/piholeCheckout.sh Outdated Show resolved Hide resolved

yubiuser dismissed rdwebdesign’s stale review via f253e11 April 23, 2023 07:40

yubiuser marked this pull request as ready for review April 23, 2023 10:16

yubiuser marked this pull request as draft April 23, 2023 10:17

Allow to checkout tags

2e07467

Signed-off-by: Christian König <[email protected]>

yubiuser force-pushed the checkout/tags branch from 01b37b8 to 2e07467 Compare April 23, 2023 12:09

Move functions to piholeCheckout.sh

d9e4e11

Signed-off-by: Christian König <[email protected]>

yubiuser marked this pull request as ready for review April 23, 2023 12:13

yubiuser requested review from a team and rdwebdesign April 23, 2023 12:13

yubiuser added Testing Required PR: Approval Required Open Pull Request, needs approval labels Apr 23, 2023

yubiuser marked this pull request as draft May 4, 2023 20:16

Use mapfile

a023195

Signed-off-by: Christian König <[email protected]>

yubiuser marked this pull request as ready for review May 5, 2023 12:51

DL6ER reviewed May 10, 2023

View reviewed changes

advanced/Scripts/update.sh Outdated Show resolved Hide resolved

automated install/basic-install.sh Outdated Show resolved Hide resolved

Apply suggestions from code review

ff91db7

Co-authored-by: DL6ER <[email protected]> Signed-off-by: yubiuser <[email protected]>

DL6ER reviewed May 12, 2023

View reviewed changes

yubiuser force-pushed the checkout/tags branch from 09a3fc8 to 51e55b7 Compare May 13, 2023 08:11

Add warning on checkout core tags <v5.17

c317050

Signed-off-by: Christian König <[email protected]>

yubiuser force-pushed the checkout/tags branch from 51e55b7 to c317050 Compare May 13, 2023 09:15

DL6ER requested a review from dschaper May 14, 2023 07:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow to checkout tags via `pihole checkout` #5259

Allow to checkout tags via `pihole checkout` #5259

yubiuser commented Apr 21, 2023 •

edited

rdwebdesign commented Apr 21, 2023 •

edited

yubiuser commented Apr 23, 2023

DL6ER left a comment

DL6ER left a comment

DL6ER May 12, 2023

DL6ER May 12, 2023

DL6ER commented May 12, 2023

yubiuser commented May 12, 2023

DL6ER commented May 12, 2023

yubiuser commented May 12, 2023

DL6ER commented May 13, 2023

yubiuser commented May 13, 2023

yubiuser commented May 13, 2023

pralor-bot commented May 28, 2023

Allow to checkout tags via pihole checkout #5259

Are you sure you want to change the base?

Allow to checkout tags via pihole checkout #5259

Conversation

yubiuser commented Apr 21, 2023 • edited

rdwebdesign commented Apr 21, 2023 • edited

yubiuser commented Apr 23, 2023

DL6ER left a comment

Choose a reason for hiding this comment

DL6ER left a comment

Choose a reason for hiding this comment

DL6ER May 12, 2023

Choose a reason for hiding this comment

DL6ER May 12, 2023

Choose a reason for hiding this comment

DL6ER commented May 12, 2023

yubiuser commented May 12, 2023

DL6ER commented May 12, 2023

yubiuser commented May 12, 2023

DL6ER commented May 13, 2023

yubiuser commented May 13, 2023

yubiuser commented May 13, 2023

pralor-bot commented May 28, 2023

Allow to checkout tags via `pihole checkout` #5259

Allow to checkout tags via `pihole checkout` #5259

yubiuser commented Apr 21, 2023 •

edited

rdwebdesign commented Apr 21, 2023 •

edited