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

update.sh code refactoring #1033

Merged
merged 12 commits into from Dec 27, 2016
Merged

update.sh code refactoring #1033

merged 12 commits into from Dec 27, 2016

Conversation

DL6ER
Copy link
Member

@DL6ER DL6ER commented Dec 24, 2016

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


Code refactoring of the existing update.sh without changing its external behavior. Internally, the behavior changes (getting completely independent of GitHub tags).

The old logic has been:

  1. Get tag from GitHub
  2. Get local tag
  3. Compare tags and update the corresponding parts, if necessary

Proposed new logic (we discussed about that):

  1. Fetch (possible) changes from origin
  2. Get local git status
  3. Update if local copy is behind remote version

Note that the new behavior is independent of tags but relies on the git log, only. Hence, it is much more reliable and trustworthy.

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

@DL6ER DL6ER added Bug: fixed Contains a bug resolution Enhancement labels Dec 24, 2016
@DL6ER DL6ER added this to the v2.11 milestone Dec 24, 2016
@DL6ER DL6ER changed the base branch from master to development December 24, 2016 14:49
@DL6ER
Copy link
Member Author

DL6ER commented Dec 24, 2016

Note that this will also make the updater work for other branches, like:

root@raspberrypi:/etc/.pihole# git pull
Already up-to-date.
root@raspberrypi:/etc/.pihole# git reset --hard HEAD^
HEAD is now at 4632b0f Updated updater logic
root@raspberrypi:/etc/.pihole# pihole -up
::: Checking for updates...
::: Pi-hole Core:   update available
::: Web Interface:  up to date
:::
::: Pi-hole core files out of date
:::
[... rest not shown ...]

Copy link
Contributor

@AzureMarker AzureMarker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we run pihole -v at the end and/or at the start so we can see what version the user is on/update to?

# Fetch latest changes and apply
git -C "${directory}" pull --quiet &> /dev/null || ${retVal}=1
return ${retVal}
git -C "${directory}" pull --quiet
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should use the long version of -C #1009

Signed-off-by: Dan Schaper <dan.schaper@pi-hole.net>
Signed-off-by: Dan Schaper <dan.schaper@pi-hole.net>
@dschaper
Copy link
Member

pihole -v would show the tag for the release, but not the hash (currently.) Ideally, the update script shouldn't output any kind of information, just a return value that the caller would identify and then let the caller print the information. update.sh shouldn't ever be run bare, it should be called as a utility script/library function. Will the user ever see any of the output from this script?

@DL6ER
Copy link
Member Author

DL6ER commented Dec 24, 2016

It is run bare for updating.

https://github.com/pi-hole/pi-hole/blob/development/pihole#L51

Signed-off-by: Dan Schaper <dan.schaper@pi-hole.net>
@dschaper
Copy link
Member

Right, it's called via a script though, we don't advise users to go to a shell prompt and run update.sh by itself.

@DL6ER
Copy link
Member Author

DL6ER commented Dec 24, 2016

Does that matter if is called via a script? Would you like to rewrite more fundamental?

@dschaper
Copy link
Member

I'm over on MM now, but I think these should be utilities or the beginnings of a library and we handle all the input/output from pihole. So we keep the interface with the user in one place, instead of spreading it out. Longer explanation in chat...

@dschaper
Copy link
Member

dschaper commented Dec 24, 2016

Approved

Approved with PullApprove

@dschaper
Copy link
Member

dschaper commented Dec 27, 2016

Approved

Approved with PullApprove

@dschaper dschaper merged commit fcdd58a into development Dec 27, 2016
@DL6ER DL6ER deleted the updater_rewrite branch December 27, 2016 20:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug: fixed Contains a bug resolution Enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants