-
Notifications
You must be signed in to change notification settings - Fork 383
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
[#4072] improvement(docker-image): Transfer docker hub from datastrato to apache #4523
Conversation
@mchades |
@xunliu |
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.
Should this file be removed or updated?https://github.com/apache/gravitino/blob/main/docs/publish-docker-images.md
done |
dev/docker/trino/Dockerfile
Outdated
@@ -67,7 +67,7 @@ RUN mkdir /tmp/gravitino | |||
COPY --chown=trino:trino packages/gravitino-trino-connector /tmp/gravitino | |||
|
|||
ARG IMAGE_NAME | |||
RUN if [ "$IMAGE_NAME" = "datastrato/trino" ] ; then \ | |||
RUN if [ "$IMAGE_NAME" = "apache/gravitino-*:trino-*" ] ; then \ |
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.
@diqiu50
Please help to verify this part.
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 this this is better:
apache/gravitino-trino-*:*
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.
After this PR, the name for Trino Docker image will be:
apache/gravitino-ci:trino-xxxx
apache/gravitino-playground:trino-xxxx
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.
@diqiu50
We will use the tag name to indicate whether it's a Trino image, so please help verify its accuracy.
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.
That's ok.
- 'gravitino-ci:ranger' | ||
- 'gravitino-playground:trino' | ||
- 'gravitino-playground:hive' | ||
- 'gravitino-playground:ranger' | ||
- 'gravitino-iceberg-rest-server' |
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.
Should you update this?
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.
It's the display name in GitHub image actions, I've already updated the final image name. Please refer to line 84.
docs/docker-image-details.md
Outdated
- trino:435-gravitino-0.5.1 | ||
- Based on Gravitino 0.5.1, you can know more information from 0.5.1 release notes. | ||
- apache/gravitino-playground:trino-435-gravitino-0.6.0-incubating (Switch to Apache official DockerHub repository) | ||
- Use `datastrato/trino:435-gravitino-0.5.1` Dockerfile to rebuild the image. |
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.
Is this comment right?
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.
Yes, I will modify it as all the docker images are based on the release-0.6 NOT 0.5.1
@@ -112,28 +112,32 @@ You can use these kinds of Docker images to facilitate integration testing of al | |||
You can use this kind of image to test the catalog of Apache Hive with kerberos enable | |||
|
|||
Changelog | |||
- gravitino-ci-kerberos-hive:0.1.5 | |||
|
|||
- apache/gravitino-ci:kerberos-hive-0.1.5 (Switch to Apache official 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.
Maybe we would better use 0.1.6 here.
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.
We can keep the current version as it is because they are now different Docker repos.
After you release the images, you should update the playground, too. |
@jerqi do you have any more feedbacks? |
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.
Generally LGTM, @jerqi can you please take another look?
…astrato to apache (apache#4523) ### What changes were proposed in this pull request? Transfer docker image used Gravitino from user `datastrato` to Apache. ### Why are the changes needed? Apache project should use the apache docker hub as the user to keep docker image. Fix: apache#4072 ### Does this PR introduce _any_ user-facing change? N/A. ### How was this patch tested? Existing CI. --------- Co-authored-by: xunliu <[email protected]>
What changes were proposed in this pull request?
Transfer docker image used Gravitino from user
datastrato
to Apache.Why are the changes needed?
Apache project should use the apache docker hub as the user to keep docker image.
Fix: #4072
Does this PR introduce any user-facing change?
N/A.
How was this patch tested?
Existing CI.