Publish latest construct version to the website #473
Conversation
Signed-off-by: Ritesh Karankal <[email protected]>
Signed-off-by: Ritesh Karankal <[email protected]>
|
Yay, your first pull request! 👍 A contributor will be by to give feedback soon. In the meantime, you can find updates in the #github-notifications channel in the community Slack. |
Summary of ChangesHello @ritesh-karankal, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new automation step to dynamically publish the latest Meshery construct schema versions to the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new function to generate-golang.sh that automatically updates index.md with the latest construct links. This is a great addition for keeping documentation up-to-date. The overall logic is sound, but I've identified a critical issue with path resolution in the script that needs to be addressed. I've also included several suggestions to improve the script's robustness and maintainability by adhering to shell scripting best practices. Please take a look at the detailed comments.
| local script_dir root_dir src out_md tmp_file map_file | ||
| script_dir="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" | ||
| root_dir="$(cd "$script_dir/.." && pwd)" | ||
| src="$root_dir/generate-golang.sh" |
There was a problem hiding this comment.
The script name generate-golang.sh is hardcoded. It's better to use $(basename "${BASH_SOURCE[0]}") to refer to the current script, making it more robust if the script is ever renamed.
| src="$root_dir/generate-golang.sh" | |
| src="$root_dir/$(basename "${BASH_SOURCE[0]}")" |
|
|
||
| tmp_file="$(mktemp)" | ||
| map_file="$(mktemp)" | ||
| trap 'rm -f "$tmp_file" "$map_file"' RETURN |
There was a problem hiding this comment.
The trap is set for RETURN, which only triggers on a normal function return. If the script exits due to an error (because of set -e) or is interrupted (e.g., with Ctrl+C), the temporary files will not be cleaned up. Using EXIT is more robust as it covers any script exit.
| trap 'rm -f "$tmp_file" "$map_file"' RETURN | |
| trap 'rm -f "$tmp_file" "$map_file"' EXIT |
| echo "|---|---:|---|" >> "$tmp_file" | ||
|
|
||
| # Extract mapping from generate_schema_models | ||
| perl -nle 'while(/generate_schema_models\s+"([^\"]+)"\s+"([^\"]+)"/g){print "$1|$2"}' "$src" > "$map_file" |
There was a problem hiding this comment.
This perl command parses the script itself to extract arguments from generate_schema_models calls. This is quite fragile and can easily break if the function calls are refactored, or even if spacing or quoting changes.
A more robust approach would be to define this mapping in a data structure, like a bash associative array, and then iterate over it both for calling generate_schema_models and for generating this markdown file. This would centralize the data and make the script more maintainable.
| done < "$map_file" | ||
|
|
||
| # Include JSON-only constructs (directories that lack an OpenAPI bundle) | ||
| find "$root_dir/schemas/constructs" -type f -iname '*.json' | sort | while IFS= read -r f; do |
There was a problem hiding this comment.
Piping the output of find to a while read loop is not fully robust for filenames that might contain newlines or other special characters. A safer and more standard practice is to use find with -print0 and while with read -d ''.
| find "$root_dir/schemas/constructs" -type f -iname '*.json' | sort | while IFS= read -r f; do | |
| find "$root_dir/schemas/constructs" -type f -iname '*.json' -print0 | sort -z | while IFS= read -d '' -r f; do |
| awk -v s="$start" -v e="$end" -v t="$tmp_file" ' | ||
| BEGIN{inside=0} | ||
| { if(index($0,s)){ print; system("cat " t); inside=1; next } | ||
| if(inside){ if(index($0,e)){ print; inside=0 } else next } | ||
| else print } | ||
| ' "$out_md" > "$out_md.tmp" && mv "$out_md.tmp" "$out_md" |
There was a problem hiding this comment.
The awk script shells out to cat to insert the file content. This is generally considered bad practice as it's less efficient and can be a security risk if the filename variable isn't properly sanitized (it is safe here since it comes from mktemp, but it's a good habit to avoid it).
A more robust and idiomatic awk approach is to read the replacement file's content into a variable and then print it, avoiding the system() call.
| awk -v s="$start" -v e="$end" -v t="$tmp_file" ' | |
| BEGIN{inside=0} | |
| { if(index($0,s)){ print; system("cat " t); inside=1; next } | |
| if(inside){ if(index($0,e)){ print; inside=0 } else next } | |
| else print } | |
| ' "$out_md" > "$out_md.tmp" && mv "$out_md.tmp" "$out_md" | |
| awk -v s="$start" -v e="$end" ' | |
| NR==FNR { r = r $0 RS; next } | |
| index($0, s) { print; printf "%s", r; p=1; next } | |
| p && index($0, e) { print; p=0; next } | |
| p { next } | |
| { print } | |
| ' "$tmp_file" "$out_md" > "$out_md.tmp" && mv "$out_md.tmp" "$out_md" |
1 participant
Notes for Reviewers
This PR fixes #412
Adds a publish latest link function to the script that publishes (or updates) the 'latest' construct version and path to the website so users always resolve to the most recent latest schema version.
Signed commits