-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
feat: add script to automatically publish readme to docker hub #6118
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Meet Soni <[email protected]>
Signed-off-by: Meet Soni <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6118 +/- ##
==========================================
- Coverage 96.48% 96.45% -0.03%
==========================================
Files 352 352
Lines 19973 20007 +34
==========================================
+ Hits 19270 19297 +27
- Misses 520 526 +6
- Partials 183 184 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Signed-off-by: Meet Soni <[email protected]>
@@ -107,6 +107,7 @@ else | |||
if [[ "$overwrite" == 'N' ]]; then | |||
check_overwrite "${IMAGE_TAGS[@]}" | |||
fi | |||
bash scripts/upload-docker-readme.sh "${component_name}" "${dir_arg}"/README.md |
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.
+1 for having separate script!
I would prefer this to be called after the Docker image upload was successful. You can set a flag here and call the script after docker buildx build
below
readme_path="$2" | ||
abs_readme_path=$(realpath "$readme_path") | ||
|
||
DOCKERHUB_TOKEN=${DOCKERHUB_TOKEN:-} |
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.
DOCKERHUB_TOKEN=${DOCKERHUB_TOKEN:-} | |
DOCKERHUB_TOKEN=${DOCKERHUB_TOKEN:?'missing Docker Hub token'} |
|
||
DOCKERHUB_TOKEN=${DOCKERHUB_TOKEN:-} | ||
dockerhub_repository="jaegertracing/$repo" | ||
dockerhub_url="https://hub.docker.com/v2/repositories/$dockerhub_repository/" |
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.
Q: what is the story with the other registries? E.g. we also push to quay.io - does it support similar API for updating the readme?
# Copyright (c) 2024 The Jaeger Authors. | ||
# SPDX-License-Identifier: Apache-2.0 | ||
set -euxf -o pipefail | ||
repo="$1" |
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.
nit: add check for number of arguments and log error / usage if args missing
|
||
body=$(jq -n \ | ||
--arg full_desc "$readme_content" \ | ||
'{full_description: $full_desc}') |
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.
nice!
|
||
set +x | ||
|
||
body=$(jq -n \ |
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.
body=$(jq -n \ | |
# encode readme as properly escaped JSON | |
body=$(jq -n \ |
-H "Authorization: JWT $DOCKERHUB_TOKEN" \ | ||
-d "$body") | ||
|
||
dockerhub_status_code="$dockerhub_response" |
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.
what are you trying to achieve with this assignment?
--arg full_desc "$readme_content" \ | ||
'{full_description: $full_desc}') | ||
|
||
dockerhub_response=$(curl -s -o /dev/null -w "%{http_code}" -X PATCH "$dockerhub_url" \ |
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.
-o /dev/null
Will it not swallow the output / response, especially in case of an error?
echo "Successfully updated Docker Hub README for $dockerhub_repository" | ||
else | ||
echo "Failed to update Docker Hub README for $dockerhub_repository with status code $dockerhub_status_code" | ||
echo "Full response: $dockerhub_response" |
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.
can you include in the description a test that hits this point, to show how the logs look?
Which problem is this PR solving?
Fixes: #3842
Description of the changes
How was this change tested?
Checklist
jaeger
:make lint test