Multiple Repositories for Addons #539

sean-e-dietrich · 2018-05-21T06:50:36Z

Started development for multiple repositories that can be used for addons.
Additionally, added ability to run updates between addons. Rather then
running the risk of the addons being out of sync

This is a rewrite of the PR #535 and I believe this solves #368 and solves #405

This takes information that was written through back channels and allows for GIT repos to be used when checking out addons instead of downloading raw files with only support for github.

Additionally, support for multiple repositories is added. The repositories must be prefixed with DOCKSAL_ADDONS_REPO_ in order to have them searched. The DOCKSAL_ADDONS_REPO which is the same repo at https://github.com/docksal/addons is searched first but can be overridden within the .docksal/docksal.env file.

Syntax:

DOCKSAL_ADDONS_REPO_[NAME]="[Repo URL]|[Branch]"

Because a repository can have multiple branches there is an additional options for the variable to designate which branch is selected.

[NAME] Should be replaced with a unique machine name for that repo
[Repo URL] Should be the GIT repo url that will be used to clone the repo.
[BRANCH] Is optional but designates what branch of the repo to use, if not included master branch will be used.

NOTE A pipe (|) is used to separate the [REPO URL] and [BRANCH]. If [BRANCH] is omitted then the pipe is not necessary

Examples:

 # Checks master branch
DOCKSAL_ADDONS_REPO_TOOLKIT="https://github.com/kanopi/docksal-toolkit.git"
# Checks sample branch
DOCKSAL_ADDONS_REPO_TOOLKIT_SAMPLE="https://github.com/kanopi/docksal-toolkit.git|sample"

This also adds in a fin addon update command which will allow for the addon to run a similar process as the fin addon install. The update process will look for pre-update and post-update hooks as well.

The purpose for this is that if an addon has a single repo it can be tracked and to a certain extent versioned appropriately, but more so then that, if there is a need for developer to use their own personal addons in projects, but they also have a repo which is shared amongst a team then they can use both of the repositories.

Started development for multiple repositories that can be used for addons. Additionally, added ability to run updates between addons. Rather then running the risk of the addons being out of sync

achekulaev · 2018-05-22T00:14:01Z

@sean-e-dietrich can you please rebase it on the develop branch that now has your commit to fix tests for external PRs?

sean-e-dietrich · 2018-05-22T00:15:01Z

@achekulaev I already merged the changes in and am rerunning the tests.

achekulaev · 2018-05-22T20:29:07Z

bin/fin

@@ -167,6 +167,7 @@ URL_ADDONS_HOSTING="https://raw.githubusercontent.com"
 URL_ADDONS_REPO="$URL_ADDONS_HOSTING/docksal/addons"
 # Remove URL_REPO_ADDONS in Nov 2018
 URL_REPO_ADDONS="https://raw.githubusercontent.com/docksal/addons"


This variable is unused now

I can remove it, I was leaving it there because I know you said remove in Nov '18

achekulaev · 2018-05-22T20:29:44Z

bin/fin

@@ -1728,11 +1729,13 @@ show_help_addon ()
 	echo
 	echo-green "Commands:"
 	printh "install <name>" "Install addon"
+    printh "update <name>" "Update addon"


Should use tab for indentation

achekulaev · 2018-05-22T20:50:26Z

bin/fin

+		config="${config}|master"
+		local REPO_URL=$(echo $config | cut -d'|' -f1)
+		local REPO_BRANCH=$(echo $config | cut -d'|' -f2)
+        local uuid=$(uuidgen)


Please use tabs everywhere for indentation

Please don't use uuidgen. I'd like to avoid relying on additional binaries when possible. Use $RANDOM bash variable. Search for /tmp/etc.exports.$RANDOM in fin to see usage example

achekulaev · 2018-05-22T20:59:50Z

bin/fin

+        local uuid=$(uuidgen)
+
+        # Check to make sure the provided Repo URL is valid and the Branch Exists
+        local valid=$(git ls-remote -h $REPO_URL --refs $REPO_BRANCH | wc -l)


git's stderr should be redirected to dev/null

since it's non-fatal I would avoid echo-error

it's better to use grep -q $REPO_BRANCH to avoid counting some random output as a valid output

# Check that provided repo URL is valid and the branch exists git ls-remote -h "$REPO_URL" --refs "$REPO_BRANCH" 2>/dev/null | grep -q "$REPO_BRANCH" if [[ "$?" != "0" ]]; then echo "$key defines an invalid repository/branch reference. Skipping..." continue else ...

achekulaev · 2018-05-22T21:03:33Z

bin/fin

+    git remote add origin $repo_url > /dev/null
+    git fetch -q origin $repo_branch > /dev/null
+    git checkout origin/${repo_branch} ${addon_name} 2> /dev/null
+    git checkout origin/${repo_branch} ${addon_name}.pre-install 2> /dev/null


Not every addon has pre-install hook. We should only check the main file

achekulaev · 2018-05-22T21:07:52Z

bin/fin

+    git init > /dev/null
+    git remote add origin $repo_url > /dev/null
+    git fetch -q origin $repo_branch > /dev/null
+    git checkout origin/${repo_branch} ${addon_name} 2> /dev/null


I think this command should actually be the last. If main addon file did check out then everything is in order. If it did not then it is an error.

Styling hint. Please don't put a space here 2>/dev/null

achekulaev · 2018-05-22T21:15:06Z

bin/fin

+		if_failed_error "Could not create $addon_path" "Check file permissions and try again"
+	fi
+
+	local tmp_addon="/tmp/${ADDON_UUID}"


Let's refactor var name to addons_repo_copy

achekulaev · 2018-05-22T21:16:00Z

bin/fin

-			if_failed_error "Download has failed" "Check log above for messages"
-		fi
+		echo-red "Addon already exists in $addon_path. Run ${NC}fin addon update ${addon_name}"
+		exit 1


achekulaev · 2018-05-22T21:20:54Z

bin/fin

+		git pull --depth=1 origin/${URL_ADDONS_BRANCH}
+		# Remove Git Repo that was cloned from.
+		rm -rf $tmp_addon/.git 2>/dev/null
+		mv -R $tmp_addon $addon_path


mv -R is an anti-pattern because of inability to properly handle errors (what failed, copy or delete?) and because it will leave leftovers if copying errors happen.
cp -R followed by rm -rf should be used instead.

achekulaev · 2018-05-22T21:22:01Z

bin/fin

+    local URL_ADDONS_REPO
+	local URL_ADDONS_BRANCH
+	local ADDON_UUID
+	_addon_search_repo $addon_name URL_ADDONS_REPO URL_ADDONS_BRANCH ADDON_UUID


This block should be moved down, below the check for addon existence

achekulaev · 2018-05-22T21:25:04Z

bin/fin

+	else
+		echo-red "Unknown error"
+		rm -rf $tmp_addon
+		exit 1


echo-error "Addon main file does not exist, is a symlink or is not set to be executable" rm -rf "$tmp_addon" >/dev/null 2>&1 return 1

achekulaev · 2018-05-22T21:26:06Z

bin/fin

+	# If repo contains multiple addon let's only download the repo we need.
+	elif [[ -d $tmp_addon/$addon_name ]] && [[ -f $tmp_addon/$addon_name/$addon_name ]] && [[ -x $tmp_addon/$addon_name/$addon_name ]]; then
+		# Remove Git Repo that was cloned from.
+		rm -rf $tmp_addon/.git 2>/dev/null


Both outputs should be redirected

rm -rf "$tmp_addon/.git" >/dev/null 2>&1

achekulaev · 2018-05-22T21:26:55Z

bin/fin

+# Update addon
+# $1 - addon name
+# $2 - optional flag to install globally
+addon_update () {


How is this function different from addon_install?

@achekulaev addon_update only handles the updating piece. I believe the goal would be to eventually make it so that there were pre-update and post-update hooks for addons. Eventually also there could be the capability of versioning.

I get that in future there might be difference, but is there any real code difference with addon_install right now? Because in PR review they look identical to me. And if they are identical right now I would just call addon_install for fin addon update

the only difference would be the hooks. So seeing as the same code is ran for them both maybe refactoring the code and putting it in a function of it's own so it isn't duplicate coding.

achekulaev · 2018-05-22T21:34:46Z

bin/fin

+		git ls-remote -h "$REPO_URL" --refs "$REPO_BRANCH" 2>/dev/null | grep -q "$REPO_BRANCH"
+		if [[ "$?" != "0" ]]; then
+			echo "$key defines an invalid repository/branch reference. Skipping..."
+			continue


There should be no continue here or rm -rf "/tmp/${uuid}" will not clean up.
Actually you might want to revisit this whole thing and move rm -rf into inside if-clauses.

achekulaev · 2018-05-22T21:37:01Z

bin/fin

+	local repo_uuid=$4
+
+	echo-green "Searching for ${addon} in predefined repositories..."
+	for key in "${repos[@]}"


for config in "${repos[@]}"

achekulaev · 2018-05-22T21:37:35Z

bin/fin

+	echo-green "Searching for ${addon} in predefined repositories..."
+	for key in "${repos[@]}"
+	do
+		local config=${!key}


no need for this, it should be for config in "${repos[@]}"

@achekulaev

repos is an array of variable names
When looping through those variables, key refers to the variable name that is being used.
config is referring to the value for the variable that is stored.

Example:

DOCKSAL_ADDONS_REPO="https://github.com/docksal/addons" DOCKSAL_ADDONS_REPO_X="https://github.com/kanopi/docksal-toolkit.git|sample" eval 'repos=(${!'"DOCKSAL_ADDONS_REPO"'@})' # should produce something like the following array # repos=(DOCKSAL_ADDONS_REPO DOCKSAL_ADDONS_REPO_X) for key in "${repos[@]}" do local config=${!key} #when looped will now grab the value for that variable done

Right. Then let's refactor to
for repo in "${repos[@]}"
key is just too generic.

achekulaev · 2018-05-22T21:38:15Z

bin/fin

+	done
+
+	if [[ -z "${REPO_URL}" ]]; then
+		echo-red "Addon not found to install."


echo-error "Could not find addon ${addon_name} in any of the pre-defined repositories." \ "Check repositories and branches spelling" return 1

achekulaev · 2018-05-24T23:38:43Z

bin/fin

@@ -5167,47 +5238,94 @@ addon_install ()
 		echo-red "Pre-install hook has failed and aborted the installation."
 		# Delete downloaded files if pre-install hook fails
 		[[ -d "$addon_path" ]] && [[ "$addon_path" =~ ".docksal" ]] &&
-			rm -r "$addon_path"


that indentation was intentional

achekulaev · 2018-05-30T21:47:27Z

@sean-e-dietrich could you give me write access to your repo fork? I'd like to download and test first and I think I will have more fixes afterwards.

sean-e-dietrich · 2018-05-31T05:58:27Z

@achekulaev I couldn't give you access to the Kanopi's org but I cloned the repo into a personal account and made it private. Also added you on the repo as a collaborator

achekulaev · 2018-05-31T14:38:14Z

Thanks. I don't need access to Kanopi.org, the access you've given to https://github.com/sean-e-dietrich/docksal which you have raised the PR from is enough.

lmakarov · 2019-10-03T23:18:40Z

Needs to be rebased into develop.

lmakarov · 2020-10-28T04:13:55Z

This PR has been open for 2.5 years. @sean-e-dietrich do you still intend finalizing it? If not, please close.

sean-e-dietrich added 2 commits May 20, 2018 23:30

Multiple Repositories for Addons

e30124d

Started development for multiple repositories that can be used for addons. Additionally, added ability to run updates between addons. Rather then running the risk of the addons being out of sync

Merge remote-tracking branch 'origin/develop' into features/addons

493c74d

sean-e-dietrich mentioned this pull request May 22, 2018

Addon Repositories Extension. #535

Closed