extensions script cleanup #316

apeschel · 2021-04-01T00:47:15Z

Set pipefail
Explicitly define all external variables used in this script
Remove extraneous whitespace
Correct shellcheck issues
Consistently use shell builtins for logic
Lower case for local variable names
Better variable name for failed downloads

apeschel · 2021-04-01T00:48:33Z

Some cleanup I did while debugging this script for keycloak/keycloak-operator#340

On a side note, it appears KEYCLOAK_EXTENSIONS was switched to KEYCLOAK_EXTENSION accidentally in the past:

da4b38c#diff-51b1ab20d6e750a4464ec80bb5021a47be580d89ccc4cd25427ee812c5893347L33-R44

vmuzikar · 2021-05-18T06:30:34Z

@apeschel Thank you for your contribution. Could you please create a JIRA and squash your commits? See the contribution guide.

vmuzikar · 2021-05-18T11:15:36Z

As this is (among other changes) fixes a critical issue, created a JIRA for it: https://issues.redhat.com/browse/KEYCLOAK-18143

vmuzikar · 2021-05-18T11:49:41Z

@iankko @drichtarik Could you please review?

iankko · 2021-05-19T07:59:52Z

keycloak-init-container/extensions.sh


-EXTENSIONS_VOLUME="${EXTENSIONS_VOLUME:-/opt/extensions}"
+shopt -so errexit pipefail


@apeschel

In which way the meaning of the previous shopt commad differs from set -xeuo pipefail? Did shellcheck suggest this form for you?

Or if the display of expanded PS4 value isn't desired, could set -euo pipefail form be used? The latter form looks a more commonly used one for me, but that's just a nuance.

@iankko shopt is a personal preference/style thing - I just find shopt has a cleaner interface than set.

I'll switch it to set since that appears to be the local preference.

@iankko I'm curious where the set -xeuo pipefail comes from though -- there was just a -e on the shebang line previously.

@iankko I'm curious where the set -xeuo pipefail comes from though -- there was just a -e on the shebang line previously.

From other scripts in this repo, e.g. here:
https://github.com/keycloak/keycloak-containers/blob/master/server/tools/docker-entrypoint.sh#L2

The -x there is just to print the expanded form of the statements to be executed. Can be removed if not desired.

iankko · 2021-05-19T08:08:55Z

keycloak-init-container/extensions.sh

    else
-        echo -e "Can not download the extension: $EXTENSION_URL\nError code: $STATUS_CODE"
-        ((STATUS+=1))
+        echo -e "Can not download the extension: $extension_url\nError code: $status_code"


... $extension_url\nError code:...

Would the above better to be enclosed in curly brackets, IOW ... ${extension_url}\nError code:... (unsure if \n character is enough Bash not to try to expand the $extension_url\nError variable instead. Maybe it is).

iankko · 2021-05-19T08:09:31Z

keycloak-init-container/extensions.sh

-        echo -e "Can not download the extension: $EXTENSION_URL\nError code: $STATUS_CODE"
-        ((STATUS+=1))
+        echo -e "Can not download the extension: $extension_url\nError code: $status_code"
+        ((FAILED_DOWNLOADS+=1))


Since changing variable names to lowercase, should this one be lowercased too?

It's being used as a global variable, which I typically leave uppercase -- would you prefer it to be lowercase?

Ok to keep it uppercase then.

iankko · 2021-05-19T08:13:21Z

keycloak-init-container/extensions.sh

 }

+mkdir -p "$EXTENSIONS_VOLUME"
+if ! cd "$EXTENSIONS_VOLUME"; then
+    echo "Cannot cd to $EXTENSIONS_VOLUME"


s/"Cannot cd to $EXTENSIONS_VOLUME"/"Failed to change directory to $EXTENSIONS_VOLUME"/g?

(error message shouldn't contain additional script commands / statements, but rather explain the reason in human language. But maybe again just issue of taste / nuance).

iankko · 2021-05-19T08:14:03Z

keycloak-init-container/extensions.sh

-STATUS=0
-for EXT in ${KEYCLOAK_EXTENSION[@]} ; do
-    download_extension "$EXT"
+FAILED_DOWNLOADS=0


If going to lowercase this one, should be lowercased here too.

iankko · 2021-05-19T08:16:24Z

keycloak-init-container/extensions.sh

-    download_extension "$EXT"
+FAILED_DOWNLOADS=0
+
+for EXT in ${KEYCLOAK_EXTENSIONS[*]}; do


This is where set -u above would help to catch this kind of variable names typo.

iankko · 2021-05-19T08:22:28Z

keycloak-init-container/extensions.sh

    echo
-    echo -e "Extensions.sh script failed at downloading all required extensions, number of failed downloads: $STATUS \n"
+    echo -e "Extensions.sh script failed at downloading all required extensions, number of failed downloads: $FAILED_DOWNLOADS \n"


s/...failed at downloading all.../...failed to download all.../g ?

Again, if FAILED_DOWNLOADS is going to be lowercased, it should be lowercased two times here (lines #58 and #60).

iankko

/LGTM Thanks, @apeschel. Just couple of comments / nuances to clarify yet.

apeschel · 2021-05-27T18:22:34Z

@iankko Thanks for the review! I've been pretty busy so it took a while to get to following up. I made all your suggested changes, with the exception of the FAILED_DOWNLOADS casing, since it's a global variable. Let me know if you would still prefer it to be lowercase.

iankko · 2021-05-31T09:08:32Z

@apeschel

@iankko Thanks for the review! I've been pretty busy so it took a while to get to following up. I made all your suggested changes, with the exception of the FAILED_DOWNLOADS casing, since it's a global variable. Let me know if you would still prefer it to be lowercase.

Thank you for the changes! Ok to keep FAILED_DOWNLOADS as uppercase then.

The change LGTM. But could you squash the three commits into one final one, yet?

Thank you.

iankko

LGTM (once the commits are squashed).

* Set pipefail * Explicitly define all external variables used in this script * Remove extraneous whitespace * Correct shellcheck issues * Consistently use shell builtins for logic * Lower case for local variable names * Better variable name for failed downloads

apeschel · 2021-06-01T17:22:47Z

@iankko sqauashed

iankko · 2021-06-03T09:07:55Z

@iankko sqauashed

@apeschel Brilliant, TYVM! One kind request yet though. The s/KEYCLOAK_EXTENSION/KEYCLOAK_EXTENSIONS/g issue got addressed within KEYCLOAK-18143 JIRA already (via #321).

That means, once you rebase this PR against the current master, you will (probably) see a conflict of the PR with the master. Could you rebase to master and fix that conflict yet?

Thanks!
Jan

apeschel · 2021-06-07T16:41:40Z

@iankko I already rebased on master when I squashed, there no merge conflict with master :)

iankko · 2021-06-08T08:55:28Z

@iankko I already rebased on master when I squashed, there no merge conflict with master :)

@apeschel Ok, great. Thanks!

twobiers · 2022-04-04T14:50:57Z

keycloak-init-container/extensions.sh

-            FILENAME="$F"
+        if ! grep -q -i '^< content-disposition:.*filename=' /tmp/headers; then
+            local F
+            F="$(od -N8 -tx1 -An -v /dev/urandom | tr -d "").jar"


This produces a filename with whitespaces included. See #390.

Suggested change

F="$(od -N8 -tx1 -An -v /dev/urandom | tr -d "").jar"

F="$(od -N8 -tx1 -An -v /dev/urandom | tr -d " ").jar"

Suggested change

F="$(od -N8 -tx1 -An -v /dev/urandom | tr -d "").jar"

F="$(head -c8 /dev/urandom | xxd -p).jar"

stianst · 2022-10-24T07:09:07Z

With Keycloak 20 the WildFly based distribution is no longer supported. For the newer Quarkus distribution of Keycloak, check out the new documentation, or the updated container sources.

apeschel referenced this pull request May 17, 2021

refactor: extensions.sh script refactor

da4b38c

abstractj requested review from drichtarik, iankko, vmuzikar and slaskawi May 18, 2021 12:51

abstractj assigned slaskawi May 18, 2021

iankko reviewed May 19, 2021

View reviewed changes

iankko suggested changes May 31, 2021

View reviewed changes

apeschel force-pushed the script-fixes branch from 07056f7 to e48a165 Compare June 1, 2021 17:21

iankko approved these changes Jun 8, 2021

View reviewed changes

drichtarik approved these changes Nov 20, 2021

View reviewed changes

twobiers mentioned this pull request Apr 4, 2022

Random generated extension filename contains whitespaces #390

Closed

twobiers reviewed Apr 4, 2022

View reviewed changes

stianst closed this Oct 24, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

extensions script cleanup #316

extensions script cleanup #316

apeschel commented Apr 1, 2021

apeschel commented Apr 1, 2021

vmuzikar commented May 18, 2021 •

edited

Loading

vmuzikar commented May 18, 2021

vmuzikar commented May 18, 2021

iankko May 19, 2021

apeschel May 27, 2021

apeschel May 27, 2021

iankko May 31, 2021

iankko May 19, 2021

iankko May 19, 2021

apeschel May 27, 2021

iankko May 31, 2021

iankko May 19, 2021

iankko May 19, 2021

iankko May 19, 2021

iankko May 19, 2021

iankko left a comment

apeschel commented May 27, 2021

iankko commented May 31, 2021

iankko left a comment

apeschel commented Jun 1, 2021

iankko commented Jun 3, 2021

apeschel commented Jun 7, 2021

iankko commented Jun 8, 2021

twobiers Apr 4, 2022

stianst commented Oct 24, 2022


		EXTENSIONS_VOLUME="${EXTENSIONS_VOLUME:-/opt/extensions}"
		shopt -so errexit pipefail

	F="$(od -N8 -tx1 -An -v /dev/urandom \| tr -d "").jar"
	F="$(od -N8 -tx1 -An -v /dev/urandom \| tr -d " ").jar"

	F="$(od -N8 -tx1 -An -v /dev/urandom \| tr -d "").jar"
	F="$(head -c8 /dev/urandom \| xxd -p).jar"

extensions script cleanup #316

extensions script cleanup #316

Conversation

apeschel commented Apr 1, 2021

apeschel commented Apr 1, 2021

vmuzikar commented May 18, 2021 • edited Loading

vmuzikar commented May 18, 2021

vmuzikar commented May 18, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iankko left a comment

Choose a reason for hiding this comment

apeschel commented May 27, 2021

iankko commented May 31, 2021

iankko left a comment

Choose a reason for hiding this comment

apeschel commented Jun 1, 2021

iankko commented Jun 3, 2021

apeschel commented Jun 7, 2021

iankko commented Jun 8, 2021

Choose a reason for hiding this comment

stianst commented Oct 24, 2022

vmuzikar commented May 18, 2021 •

edited

Loading