#20 Miscellanous fixes
Merged 6 months ago by darknao. Opened 7 months ago by oturpe.
fedora-docs/ oturpe/template misc-fixes  into  main

file modified
+2 -2
@@ -5,8 +5,8 @@ 

  # Title will be visible on the page. 

  title: Pizza Factory # <---- PLEASE MODIFY

  

- # If you don't plan to have multiple versions of the docs (for example, to document multiple versions of some software), you can ignore this field. Otherwise, change "master" to a specific version.

- version: master

+ # If you don't plan to have multiple versions of the docs (for example, to document multiple versions of some software), you can ignore this field. Otherwise, change "~" to a specific version.

+ version: ~

  

  # We encourage you to name the index page as "index.adoc". If you absolutely have to use a different name, please reflect it here. You can ignore this field otherwise. 

  start_page: ROOT:index

file modified
+16 -15
@@ -3,16 +3,16 @@ 

  image="docker.io/antora/antora"

  cmd="--html-url-extension-style=indexify site.yml"

  

- if [ "$(uname)" == "Darwin" ]; then

+ if uname | grep -iwq darwin; then

      # Running on macOS.

      # Let's assume that the user has the Docker CE installed

      # which doesn't require a root password.

      echo ""

      echo "This build script is using Docker container runtime to run the build in an isolated environment."

      echo ""

-     docker run --rm -it -v $(pwd):/antora $image $cmd

+     docker run --rm -it -v "$(pwd):/antora" $image $cmd

  

- elif [ "$(expr substr $(uname -s) 1 5)" == "Linux" ]; then

+ elif uname | grep -iq linux; then

      # Running on Linux.

      # there isn't an antora/aarch64 container, antora can be installed locally

      # Check whether podman is available, else faill back to docker
@@ -20,13 +20,13 @@ 

  

      if [ -f /usr/local/bin/antora ]; then

          /usr/local/bin/antora $cmd

-     elif [[ `uname -m` == "aarch64" ]]; then

+     elif uname -m | grep -iwq aarch64; then

          echo "no antora/aarch64 container try just \`npm install -g @antora/cli @antora/site-generator-default\`"

      elif [ -f /usr/bin/podman ]; then

          echo ""

          echo "This build script is using Podman to run the build in an isolated environment."

          echo ""

-         podman run --rm -it -v $(pwd):/antora:z $image $cmd

+         podman run --rm -it -v "$(pwd):/antora:z" $image $cmd

  

      elif [ -f /usr/bin/docker ]; then

          echo ""
@@ -34,18 +34,19 @@ 

          echo ""

  

          if groups | grep -wq "docker"; then

- 	    docker run --rm -it -v $(pwd):/antora:z $image $cmd

- 	else

+             docker run --rm -it -v "$(pwd):/antora:z" $image $cmd

+         else

+             echo "You might be asked for your password."

+             echo "You can avoid this by adding your user to the 'docker' group,"

+             echo "but be aware of the security implications."

+             echo "See https://docs.docker.com/install/linux/linux-postinstall/"

              echo ""

-             echo "This build script is using $runtime to run the build in an isolated environment. You might be asked for your password."

-             echo "You can avoid this by adding your user to the 'docker' group, but be aware of the security implications. See https://docs.docker.com/install/linux/linux-postinstall/."

-             echo ""

-             sudo docker run --rm -it -v $(pwd):/antora:z $image $cmd

- 	fi

+             sudo docker run --rm -it -v "$(pwd):/antora:z" $image $cmd

+         fi

      else

          echo ""

- 	echo "Error: Container runtime haven't been found on your system. Fix it by:"

- 	echo "$ sudo dnf install podman"

- 	exit 1

+         echo "Error: Container runtime haven't been found on your system. Fix it by:"

+         echo "$ sudo dnf install podman"

+         exit 1

      fi

  fi

Miscellaneous fixes from reviewing package-maintainer-docs/pull-request/53
which imported a new version of this template to package-maintainer-docs.
See individual commits for fix descriptions.
Has some overlap with #19, feel free to mix and match.

5 new commits added

  • Use `version: ~` in antora.yml
  • Fix intendation in build.sh error branch
  • Fix the Linux docker branch of build.sh
  • Replace deprecated backticks with supported $()
  • Fix quotes in build.sh
7 months ago

This looks great. I've rebased my changes on top of yours in https://pagure.io/fedora-docs/template/pull-request/19. I think we should merge this one and then mine.

Looks good, and I've only one comment:
The use of a null value (~) for version has some side effects on the UI.
The version key is currently used in the component version selector (that thing at the bottom of the left panel), and doesn't look great for unversioned components (see https://phunk.me/K8BYGY.png)
In this example, the first component is using ~, and the second : master.

One way to fix this is to modify our UI theme to use the display_version key instead, which conveniently fallback to default if version has a null value (see https://phunk.me/BV0JPV.png)

The quoting on line 9 of build.sh doesn't look right to me.

The quoting on line 9 of build.sh doesn't look right to me.

Why so? It works correctly for all output of $(uname -s) I can think of.
(But I was testing with $(echo foo bar) and such, to generate test cases.)

Probably this change could be dropped with no harm,
the inner quotes only help if uname -s returns a string with inner whitespace:

This works:

$ if [ "$(expr substr "$(echo Linux with suffix)" 1 5)" == "Linux" ]; then echo "hit"; fi
hit

This does not:

$ if [ "$(expr substr $(echo Linux with suffix) 1 5)" == "Linux" ]; then echo "hit"; fi
expr: syntax error: unexpected argument ”1”

Looks good, and I've only one comment:
The use of a null value (~) for version has some side effects on the UI.
The version key is currently used in the component version selector (that thing at the bottom of the left panel), and doesn't look great for unversioned components (see https://phunk.me/K8BYGY.png)
In this example, the first component is using ~, and the second : master.

One way to fix this is to modify our UI theme to use the display_version key instead, which conveniently fallback to default if version has a null value (see https://phunk.me/BV0JPV.png)

Thank you for noticing this,
I was completetly unaware of this issue.
What do you suggest?
Just applying your suggested fix to the UI theme,
or leaving version: master for now?

I tried to go and submit a pull request to the UI theme myself,
but I have problems working with that repository.

Maybe we can rename master to main for now and fix the theme after.

Why so? It works correctly for all output of $(uname -s) I can think of.
(But I was testing with $(echo foo bar) and such, to generate test cases.)

It is just that $(uname -s) is technically unquoted and something clever could still cause problems. For example:

if [ "$(expr substr "$(echo O'Linux with suffix)" 1 5)" == "Linux" ]; then echo matched; fi

That said, I cannot really imagine uname returning something that would cause problems.

My personal preference would have been to use grep instead of test and expr. That is:

if echo "O'Linux with suffix" | grep -q -i ^linux; then echo matched; fi

In this specific case, that would be:

if uname -s | grep -q -i ^linux; then echo matched; fi

Just my 2¢.

Maybe we can rename master to main for now and fix the theme after.

I would be more inclined to just leave it as it is, and limit the number of changes we would have to make to all sub repos. master is still a valid (but deprecated) value in 3.0.0, so I don't think we need to rush it.
I'll make a PR on the UI repo and see if we can get it merged before this one.

edit: https://pagure.io/fedora-docs/fedora-docs-ui/pull-request/55

rebased onto e43e08a

7 months ago

It is just that $(uname -s) is technically unquoted and something clever could still cause problems. For example:

if [ "$(expr substr "$(echo O'Linux with suffix)" 1 5)" == "Linux" ]; then echo matched; fi

That said, I cannot really imagine uname returning something that would cause problems.

My personal preference would have been to use grep instead of test and expr. That is:

if echo "O'Linux with suffix" | grep -q -i ^linux; then echo matched; fi

In this specific case, that would be:

if uname -s | grep -q -i ^linux; then echo matched; fi

Just my 2¢.

Agreed, even the original one was probably good for all cases that can be encountered.
But, since we started with this,
I went ahead and changed all the if [ $(uname) == "Something" ]'s to use pipe and grep.

I'll make a PR on the UI repo and see if we can get it merged before this one.

edit: https://pagure.io/fedora-docs/fedora-docs-ui/pull-request/55

PR has been merged, deployed on staging environment, and soon to be on prod.
It's all good on my side :thumbsup:

Pull-Request has been merged by darknao

6 months ago
Metadata