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

Add pihole checkout ftl #1567

Merged
merged 8 commits into from Jul 17, 2017

Conversation

Projects
None yet
5 participants
@DL6ER
Copy link
Member

DL6ER commented Jun 27, 2017

By submitting this pull request, I confirm the following (please check boxes, eg [X]) Failure to fill the template will close your PR:

Please submit all pull requests against the development branch. Failure to do so will delay or deny your request

  • I have read and understood the contributors guide.
  • 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.

How familiar are you with the codebase?:

10


Add ftl choice for pihole checkout. A lot of code had to be added as ftl is a different thing than core and web as it is not a local repository.

This template was created based on the work of udemy-dl.

@DL6ER DL6ER added the Enhancement label Jun 27, 2017

@Mcat12
Copy link
Member

Mcat12 left a comment

LGTM

@WaLLy3K
Copy link
Collaborator

WaLLy3K left a comment

There's an inconsistency between the output of pihole checkout core/web and pihole checkout ftl, both if a valid, or invalid branch is specified.

Example of an invalid branch specified:
checkout

# ARM
local rev=$(uname -m | sed "s/[^0-9]//g;")
local lib=$(ldd /bin/ls | grep -E '^\s*/lib' | awk '{ print $1 }')
if [[ "$lib" == "/lib/ld-linux-aarch64.so.1" ]]; then

This comment has been minimized.

@WaLLy3K

WaLLy3K Jun 29, 2017

Collaborator

Question: should we not be quoting and bracing strings to be consistent with the rest of the codebase? (e.g: [[ "${machine}" == "arm"* || (There's a couple of instances of differently formatted conditionals, but I won't flag each line for comment in case there's a reason for this!)

This comment has been minimized.

@dschaper

dschaper Jun 29, 2017

Member

Has this been run through a manual ShellCheck.net scan?

This comment has been minimized.

@DL6ER

DL6ER Jun 29, 2017

Author Member

@WaLLy3K I use exactly the same code as is used in the installer script (where we know that it is working fine for all users).

@dschaper Yes, it reported two minor things (1. Add a check for failing cd, 2. Don't declare variable as local and assign value in the same line)

@DL6ER

This comment has been minimized.

Copy link
Member Author

DL6ER commented Jul 4, 2017

@WaLLy3K You requested changes. How should they look like? Checking out FTL is basically another thing, as it is not a repository.

@WaLLy3K

This comment has been minimized.

Copy link
Collaborator

WaLLy3K commented Jul 5, 2017

Instead of saying "Branch [blank space] doesn't exist", could it match existing behaviour by saying "Requested Branch "" is not available" and then list the available branches? There's no reason why the output should be any different to Core or Web.

Optionally, I'd love to see the conditional formatting made consistent (quoting and bracing), but that's just me being extremely nit-picky, and will happily approve this even without this minor change! :)

PromoFaux and others added some commits Jul 16, 2017

Merge branch 'development' into tweak/checkout_FTL
Signed-off-by: Adam Warner <adamw@rner.email>

# Conflicts:
#	advanced/Scripts/piholeCheckout.sh
Fixed minor formatting issues
* Removed useless echo
* Quoted and braced conditionals
* Explicit escaping of newline
* Fixed arrays implicitly concatenating (SC2199)
* Fixed incorrect variable used in checkout web
* Add helptext for `pihole checkout ftl`
* Only attempt to install FTL if branch was found
* ~~corebranches~~ webbranches (web branches now actually listed)

Signed-off-by: Adam Warner <adamw@rner.email>
Merge branch 'tweak/checkout_FTL' of https://github.com/pi-hole/pi-hole
… into tweak/checkout_FTL

Signed-off-by: Adam Warner <adamw@rner.email>

# Conflicts:
#	advanced/Scripts/piholeCheckout.sh
@WaLLy3K

This comment has been minimized.

Copy link
Collaborator

WaLLy3K commented Jul 16, 2017

Just removing my review blocker, rather than explicitly approving it (but there's no difference according to GitHub). It'd still be good if the text of pihole checkout ftl was to look the same as pihole checkout core, but I understand that there's some limitations that prevent the actual branch readout.

list availible branches for FTL
Signed-off-by: Adam Warner <adamw@rner.email>
@PromoFaux

This comment has been minimized.

Copy link
Member

PromoFaux commented Jul 16, 2017

Confirmed working, with the exception of switching back to master branch. But I don't think that is an issue with the code, rather an issue with the .sha1 on ftl.pi-hole.net for the master branch files:

fap@fappotron:~$ pihole checkout ftl master
  Please note that changing branches 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

  [✓] Detected x86_64 architecture
  [✓] Branch master exists
  [i] Installing FTL...sha1sum: pihole-FTL-linux-x86_64.sha1: No such file or directory
  [✗] Installing FTL
  Error: Download of binary from ftl.pi-hole.net failed

@PromoFaux PromoFaux merged commit 20b2dd6 into development Jul 17, 2017

5 checks passed

codacy/pr Good work! A positive pull request.
Details
code-review/pullapprove Approved by DL6ER, PromoFaux
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details

@PromoFaux PromoFaux deleted the tweak/checkout_FTL branch Jul 17, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.