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
Allow to checkout tags via pihole checkout
#5259
base: development
Are you sure you want to change the base?
Conversation
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 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: 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) |
Signed-off-by: Christian König <ckoenig@posteo.de>
Signed-off-by: Christian König <ckoenig@posteo.de>
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 |
Signed-off-by: Christian König <ckoenig@posteo.de>
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.
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
Co-authored-by: DL6ER <DL6ER@users.noreply.github.com> Signed-off-by: yubiuser <ckoenig@posteo.de>
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.
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.
# 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 |
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 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)
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 |
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.
See my comment in update.sh
, this is the same code.
Well, I hit
|
The failure is expected as you only replaced the files in pi-hole/advanced/Scripts/piholeCheckout.sh Lines 282 to 283 in ff91db7
The old file is missing the handling of pi-hole/automated install/basic-install.sh Lines 488 to 492 in ff91db7
|
I also replaced them in the repo (and committed that to
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? |
I fear you are right. Checking out any tag before this is merged and tagged will fail on |
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 |
It would be quite some code.... I tried yesterday: we would separate getting |
|
Signed-off-by: Christian König <ckoenig@posteo.de>
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 |
What does this PR aim to accomplish?:
Allow to checkout specific tags via
pihole checkout
forcore
andweb
. So far only branches were allowed.P.S. Checking out tags for
FTL
was 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?:
--tags
added when getting upstream references.ref
is a branch or atag
and depending on that we create a new branch with or without--tracking
set (which is set by default when usingcheckout
without-b/B
)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)basic-install.sh
topiholeCheckout.sh
as they have been used only there and nowhere else.By submitting this pull request, I confirm the following:
git rebase
)