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

Fixed oca generation bug creating invalid JSON #81

Merged
merged 5 commits into from
Sep 25, 2023

Conversation

wadeking98
Copy link
Contributor

Previously the JSON files generated by the gen_ocabundlesjson.sh script would be invalid due to a trailing comma. This caused BC Wallet to fail to properly handle the JSON. I've added functionality to not add a comma on the last entries in the json file.

Signed-off-by: wadeking98 <wkingnumber2@gmail.com>
Copy link
Member

@jleach jleach left a comment

Choose a reason for hiding this comment

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

I've added a few comments. I think because its fairly long and a little complicated to read breaking it into a few functions might make it more readable or maintainable:

https://linuxize.com/post/bash-functions/

if [[ $last_schema = true && $curr_idx -eq $last_idx ]]; then
delim=""
fi
echo "{ \"org\": \"${ORG}\", \"name\": \"${NAME}\", \"desc\": \"${DESC}\", \"type\": \"${TYPE}\", \"ocabundle\": \"${BUNDLE_PATH}\" }${delim}" >>${OCALISTJSON}
Copy link
Member

Choose a reason for hiding this comment

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

Use printf instead of echo for better control over formatting and escaping. For example, instead of echo -e "{" >${OCAIDSJSON}, you can use printf '{\n' > ${OCAIDSJSON}.

if [[ $last_schema = true && $curr_idx -eq $last_idx ]]; then
delim=""
fi
echo "\"${id}\": { \"path\": \"${BUNDLE_PATH}\" }${delim}" | sed "s/~/ /g" >>${OCAIDSJSON}
Copy link
Member

Choose a reason for hiding this comment

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

Use jq instead of manually constructing JSON strings. jq is a command-line tool for processing JSON data, and it can be used to construct complex JSON objects and arrays. For example, instead of manually constructing the JSON object for each bundle, you can use jq to construct the object.

 jq -n --arg id "$id" --arg bundle_path "$BUNDLE_PATH" '{($id): {path: $bundle_path}}' >> ${OCAIDSJSON}

Copy link
Contributor

Choose a reason for hiding this comment

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

That’s a lot to eliminate the last line “,” :-). Painful…but good stuff.

And I thought most JSON processors handle an extra comma at the end of the list.

@@ -34,13 +34,32 @@ fi
echo -e "{" >${OCAIDSJSON}
echo -e "[" >${OCALISTJSON}

declare -a schema_files
schema_files=(OCABundles/schema/*)
Copy link
Member

Choose a reason for hiding this comment

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

Use readarray instead of declare -a to read an array from a command output. For example, instead of declare -a schema_files; schema_files=(OCABundles/schema/*), you can use readarray -t schema_files < <(find OCABundles/schema -type f).

schema_files=()
readarray -t schema_files < <(find OCABundles/schema -type f)

Signed-off-by: wadeking98 <wkingnumber2@gmail.com>
Signed-off-by: wadeking98 <wkingnumber2@gmail.com>
Signed-off-by: wadeking98 <wkingnumber2@gmail.com>
@cvarjao
Copy link
Member

cvarjao commented Sep 20, 2023

@amanji , @swcurran , can we get this reviewed/merged?

@swcurran
Copy link
Contributor

Sorry about that. It keeps getting bumped from my list. I’ll try for tomorrow EOD.

Copy link
Contributor

@swcurran swcurran left a comment

Choose a reason for hiding this comment

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

Looks good — just the two typos and links to be changed, and we can merge this.

@swcurran
Copy link
Contributor

Sorry for the long delay — had it on my list, but just not getting to it.

Signed-off-by: wadeking98 <wkingnumber2@gmail.com>
@wadeking98 wadeking98 requested a review from swcurran September 22, 2023 23:27
@swcurran
Copy link
Contributor

@jleach — are your suggestions required for merging? Can’t merge until you say OK.

FYI - a suggestion on a probably easier way to make this change, and a reasonable approach in any Bash scripts. The entire JSON output could have been passed through another sed script with a simple substitution regex to remove the trailing comma on the second last line. Bash pipelines are easy to do.

@jleach jleach self-requested a review September 25, 2023 16:06
@wadeking98 wadeking98 merged commit 0f56f04 into bcgov:main Sep 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants