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.

Sign up for GitHub

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

Jump to bottom

Change FTLcheckUpdate logic #5190

Open
wants to merge 1 commit into
base: development
Choose a base branch
from
Open

Change FTLcheckUpdate logic #5190

wants to merge 1 commit into from

Conversation

rdwebdesign
Copy link
Member

What does this PR aim to accomplish?

Fix #5189

How does this PR accomplish the above?

Changing the logic of the function:

  • execute curl in a separate step
  • use api.github.com and jq to retrieve tag_name
  • show the error message if the variable is empty

Link documentation PRs 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)
  6. I have checked that another pull request for this purpose does not exist.
  7. I have considered, and confirmed that this submission will be valuable to others.
  8. I accept that this submission may not be used, and the pull request closed at the will of the maintainer.
  9. 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 rdwebdesign requested review from dschaper and a team February 28, 2023 02:06
dschaper
dschaper requested changes Feb 28, 2023
View reviewed changes
Copy link
Member

@dschaper dschaper left a comment

Choose a reason for hiding this comment

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

I do think this section of code needs to be redone but I think there's a lot of missed opportunities here. We are using jq now so we should actually use it to it's abilities and not just as a dumb parser.

jq has a very rich conditionals and comparisons ability that we can use for checking the output from curl and to process that output for validity. See https://manpages.ubuntu.com/manpages/xenial/man1/jq.1.html#conditionals%20and%20comparisons

automated install/basic-install.sh

if ! FTLlatesttag=$(curl -sI https://github.com/pi-hole/FTL/releases/latest | grep --color=never -i Location: | awk -F / '{print $NF}' | tr -d '[:cntrl:]'); then
if [ -z "${FTLlatesttag}" ]; then
Copy link
Member

Choose a reason for hiding this comment

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

This will fail if there is a bad response from the curl command.

Viking:~$ tag=$(curl -s https://api.github.com/repos/pi-hole/FTL/releases/lates | jq -r .tag_name)
Viking:~$ echo $tag
null

Copy link
Member Author

Choose a reason for hiding this comment

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

I tested and never got null inside the script, but I will replace the command with:

curl -s https://api.github.com/repos/pi-hole/FTL/releases/lates | jq -r '.tag_name | values'

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you want to use jq for the test, like this?

curl -s https://api.github.com/repos/pi-hole/FTL/releases/lat | jq '.tag_name == "${FTLversion}"'

Copy link
Member Author

Choose a reason for hiding this comment

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

The current code will work for any curl response:

  • if curl returns a valid JSON, jq will try to find and return tag_name.
  • if no tag_name is found in JSON response, an empty string will be returned.
  • if curl responds with anything that is not a valid JSON, jq command will return an empty string.

Command details:

jq -sRr 'fromjson? | .tag_name | values'
  • -s will read the entire input file into a large array before apply the filters;
  • -R will not parse the input as JSON. When combined with -s, it reads the whole response into a single long string;
  • The ? is the error suppression operator and works like a simple try/catch.
  • fromjson? will try to parse the text into JSON format. If not possible will return null.
  • .tag_name will return the value of "tag_name" (or null if not found);
  • values will filter the result and return only "non null values" (returning an empty string in case of null).
  • -r will output the result as plain text (without the quotes).

@yubiuser yubiuser linked an issue Feb 28, 2023 that may be closed by this pull request
Pi-hole update treats github unavailability as FTL update available #5189
Open
@rdwebdesign
Change FTLcheckUpdate logic
1f0eb73 
- execute `curl` in a separate step
- use api.github.com and `jq` to retrieve tag_name
- check if current FTL version is less than latest
- show error message if return is empty (curl failed)

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

yubiuser commented Apr 7, 2023 •
edited 

chrko@ThinkPad-X230:~$ FTLlatesttag=$(curl -s https://api.github.com/repos/pi-hole/FTL/releases/not_available | jq -sRr 'fromjson? | .tag_name | values')
chrko@ThinkPad-X230:~$ echo $FTLlatesttag

chrko@ThinkPad-X230:~$

@dschaper
Copy link
Member

dschaper commented Apr 7, 2023

What are you showing there Yubi? Is that output expected or unexpected.

@yubiuser
Copy link
Member

yubiuser commented Apr 7, 2023

It's the expected output. When address is not reachable the output of the whole function is an empty string. As opposed to
#5190 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yubiuser yubiuser yubiuser approved these changes

@dschaper dschaper Awaiting requested review from dschaper

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Pi-hole update treats github unavailability as FTL update available
3 participants
@rdwebdesign @yubiuser @dschaper