-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Add shell script to ensure consistent version of OTEL semconv #4652
Add shell script to ensure consistent version of OTEL semconv #4652
Conversation
Signed-off-by: Harish Shan <140232061+perhapsmaple@users.noreply.github.com>
@perhapsmaple did you get any error while running |
@Mayhul-Jindal I had no errors while running make. Although, install-tools did not download the tparallel module on my machine |
@yurishkuro I wanted to know if there was any way other than using curl to determine the latest version of semconv available. The go list command only seems to work for the go.opentelemetry.io/otel package. I would be willing to work on another script that updates all references to the latest version of semconv. |
Codecov ReportPatch coverage has no change and project coverage change:
Additional details and impacted files@@ Coverage Diff @@
## main #4652 +/- ##
==========================================
- Coverage 97.04% 97.04% -0.01%
==========================================
Files 301 301
Lines 17878 17878
==========================================
- Hits 17350 17349 -1
- Misses 423 424 +1
Partials 105 105
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
… available Signed-off-by: Harish Shan <140232061+perhapsmaple@users.noreply.github.com>
scripts/check-semconv-version.sh
Outdated
@@ -10,12 +10,12 @@ done < <(find . -type f -name "*.go" -exec grep -o -H "go.opentelemetry.io/otel/ | |||
semconv_versions=($(printf "%s\n" "${semconv_map[@]}" | sort -u)) | |||
|
|||
if [ ${#semconv_versions[@]} -gt 1 ]; then | |||
echo "Error: semconv version mismatch detected" | |||
echo "Error: semconv version mismatch detected. Run ./scripts/update-semconv-version.sh to update semconv to latest version." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would recommend printing "Run <script>" line at the end of the output, after the listing of files.
scripts/check-semconv-version.sh
Outdated
declare -A semconv_map | ||
|
||
while IFS=: read -r file_name package_string; do | ||
version_number=$(echo "$package_string" | grep -o -E "v1\.[0-9]+\.[0-9]+") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
version_number=$(echo "$package_string" | grep -o -E "v1\.[0-9]+\.[0-9]+") | |
version_number=$(echo "$package_string" | grep -o -E "v[0-9]\.[0-9]+\.[0-9]+") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also, this regex is used more then once, let's move it to a variable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also, why do you need this grep here in the first place, isn't it guaranteed to match because of L8?
scripts/check-semconv-version.sh
Outdated
echo "Error: semconv version mismatch detected. Run ./scripts/update-semconv-version.sh to update semconv to latest version." | ||
{ | ||
for key in "${!semconv_map[@]}"; do | ||
printf "Source File: %-30s | Semconv Version: %s\n" "$key" "${semconv_map[$key]}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
printf "Source File: %-30s | Semconv Version: %s\n" "$key" "${semconv_map[$key]}" | |
printf "Source File: %-50s | Semconv Version: %s\n" "$key" "${semconv_map[$key]}" |
30 may not be long enough
- Introduced variables for package name and regex expression - Replaced unnecessary grep to extract version number with bash parameter expansion - Moved print "Run <script>" to the end of the output and addressed alignment Signed-off-by: Harish Shan <140232061+perhapsmaple@users.noreply.github.com>
Hi @yurishkuro ,I have addressed all of your initial comments. Let me know if there are any further changes required |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks
Which problem is this PR solving?
Resolves #4646
Description of the changes
"go.opentelemetry.io/otel/semconv/v1.xx.x" with the latest version.
How was this change tested?
I have tested the changes locally by modifying import statements
Checklist
jaeger
:make lint test
jaeger-ui
:yarn lint
andyarn test