Skip to content

Conversation

@colebow
Copy link
Member

@colebow colebow commented Jan 7, 2023

Description

So that when you search for pages that mention Docker, you end up in the right place.

Additional context and related issues

From this thread on the Trino slack: https://trinodb.slack.com/archives/CP1MUNEUX/p1672858775220719

Release notes

(x) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
( ) Release notes are required, with the following suggested text:

# Section
* Fix some things. ({issue}`issuenumber`)

@cla-bot cla-bot bot added the cla-signed label Jan 7, 2023
@colebow colebow requested review from findepi and mosabua January 7, 2023 00:00
@github-actions github-actions bot added the docs label Jan 7, 2023
Copy link
Member

@mosabua mosabua left a comment

Choose a reason for hiding this comment

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

Works for me. We could optionally update the initial paragraph to say that it is suitable for kubernetes and refer to the helm chart docs page as well

Copy link
Member

@nineinchnick nineinchnick left a comment

Choose a reason for hiding this comment

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

Docker is not the only runtime that can be used to run Trino in a container. We do publish a Docker image to Docker Hub thought.

@mosabua
Copy link
Member

mosabua commented Jan 8, 2023

@nineinchnick @colebow and @findepi .. I think we should add a sentence to the intro paragraph that says that:

  • The docker image is published to dockerhub
  • It can be used with the docker runtime, but also other runtimes
  • it is used by the helm charts for usage on k8s (and link to that page in the docs) and probably link from that page back to here..

Let's do that all in this PR since we are already on it..

@colebow
Copy link
Member Author

colebow commented Jan 9, 2023

@nineinchnick @colebow and @findepi .. I think we should add a sentence to the intro paragraph that says that:

  • The docker image is published to dockerhub
  • It can be used with the docker runtime, but also other runtimes
  • it is used by the helm charts for usage on k8s (and link to that page in the docs) and probably link from that page back to here..

Let's do that all in this PR since we are already on it..

I'm going to keep the edits minimal because of a section slightly lower down:

.. note::

   There are multiple ways to use Trino within containers. You can either run
   Trino in Docker containers locally, as explained in the following sections,
   or use a container orchestration platform like Kubernetes. For the Kubernetes
   instructions see :doc:`/installation/kubernetes`.

@colebow colebow force-pushed the colebow/docker-container-title-fix branch from 14a0b49 to d73542d Compare January 9, 2023 22:33
@mosabua
Copy link
Member

mosabua commented Jan 9, 2023

@nineinchnick @colebow and @findepi .. I think we should add a sentence to the intro paragraph that says that:

  • The docker image is published to dockerhub
  • It can be used with the docker runtime, but also other runtimes
  • it is used by the helm charts for usage on k8s (and link to that page in the docs) and probably link from that page back to here..

Let's do that all in this PR since we are already on it..

I'm going to keep the edits minimal because of a section slightly lower down:

.. note::

   There are multiple ways to use Trino within containers. You can either run
   Trino in Docker containers locally, as explained in the following sections,
   or use a container orchestration platform like Kubernetes. For the Kubernetes
   instructions see :doc:`/installation/kubernetes`.

Fair

@colebow colebow force-pushed the colebow/docker-container-title-fix branch from d73542d to b76ccb9 Compare January 10, 2023 17:53
@colebow colebow force-pushed the colebow/docker-container-title-fix branch from b76ccb9 to 411a833 Compare January 11, 2023 17:43
@colebow colebow requested a review from findepi January 24, 2023 08:11
Comment on lines +7 to +8
Docker image is published to Docker Hub and can be used with the Docker runtime,
among several others.
Copy link
Member

Choose a reason for hiding this comment

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

I don't see why we're adding this sentence. Why not just update the title?

We don't need to specify with runtimes run docker images, just like we don't really define what "a docker image" is.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it's good to be clear that this isn't the only way to do things / you don't have to follow this guide.

Copy link
Member

@mosabua mosabua Feb 1, 2023

Choose a reason for hiding this comment

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

I think this is helpful and clarifying and does not do any harm. I would leave as is and merge. Specifically also since the new title narrows the scope .. and is kind of misleading therefore, this sentence clarifies.

@colebow colebow requested a review from findepi February 6, 2023 19:04
@mosabua
Copy link
Member

mosabua commented Feb 6, 2023

@electrum @findepi could we merge this one as well?

@findepi findepi merged commit dbd5fc0 into trinodb:master Feb 6, 2023
@github-actions github-actions bot added this to the 407 milestone Feb 6, 2023
@mosabua
Copy link
Member

mosabua commented Feb 7, 2023

Thank you @findepi !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

4 participants