Skip to content
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

Simplify docker image configuration #6497

Conversation

findepi
Copy link
Member

@findepi findepi commented Jan 4, 2021

As we are breaking things anyway, this is a good occasion to clean
things up and keep only one dir as supported.

@cla-bot cla-bot bot added the cla-signed label Jan 4, 2021
@findepi findepi mentioned this pull request Jan 4, 2021
@findepi findepi force-pushed the findepi/simplify-docker-image-configuration-29b31e branch from 800cfc6 to 3b68aac Compare January 4, 2021 11:43
@@ -23,7 +23,7 @@

* Publish image as [`trinodb/trino`](https://hub.docker.com/r/trinodb/trino).
* Change base image to `azul/zulu-openjdk-centos`.
* Change configuration directories to `/usr/lib/trino/etc` and `/etc/trino`.
* Change configuration directory to `/etc/trino`.
Copy link
Member

Choose a reason for hiding this comment

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

Why are you changing release notes for past release?

Copy link
Member Author

Choose a reason for hiding this comment

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

So that they do not misguide users. I am on the fence with this though, I can retract the change.

Copy link
Member

Choose a reason for hiding this comment

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

I do not expect anyone will read the source code of release notes. But rather https://trino.io/docs/current/release/release-351.html

Copy link
Member Author

Choose a reason for hiding this comment

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

Of course, the intention would be to republish the release notes (just the same way as we did in 5ae1a10), but again I can back it out if you want me to.

Copy link
Member

Choose a reason for hiding this comment

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

Oh - I did not know we do that. Fine. Keep the change.

Copy link
Member

Choose a reason for hiding this comment

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

We don't generally update the published documentation for a specific release, but release notes are part of the documentation, so any changes for a past release note would be present when the next release is published.

For this change, since we intend to break it in 352, I would want to republish the release notes immediately.

@findepi findepi force-pushed the findepi/simplify-docker-image-configuration-29b31e branch from 3b68aac to 593644f Compare January 4, 2021 14:10
@findepi
Copy link
Member Author

findepi commented Jan 4, 2021

@losipiuk PTAL

@@ -19,7 +19,8 @@ WORK_DIR="$(mktemp -d)"
curl -o ${WORK_DIR}/trino-server-${TRINO_VERSION}.tar.gz ${SERVER_LOCATION}
tar -C ${WORK_DIR} -xzf ${WORK_DIR}/trino-server-${TRINO_VERSION}.tar.gz
rm ${WORK_DIR}/trino-server-${TRINO_VERSION}.tar.gz
cp -R bin default ${WORK_DIR}/trino-server-${TRINO_VERSION}
cp -R bin ${WORK_DIR}/trino-server-${TRINO_VERSION}
cp -R default -t ${WORK_DIR}
Copy link
Member

Choose a reason for hiding this comment

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

hmm - why so?

Copy link
Member Author

Choose a reason for hiding this comment

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

Previously the default was applied dynamically in startup script

ln -s /usr/lib/trino/default/etc /usr/lib/trino/etc

now it's statically applied just by the fact it's in the expected place in the image.

Copy link
Member

Choose a reason for hiding this comment

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

Oh yeah. I see where it is referenced now.

Copy link
Member

@losipiuk losipiuk left a comment

Choose a reason for hiding this comment

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

squash

As we are breaking things anyway, this is a good occasion to clean
things up and keep only one dir as supported.
@findepi findepi force-pushed the findepi/simplify-docker-image-configuration-29b31e branch from 593644f to 8a82b88 Compare January 4, 2021 16:25
@findepi
Copy link
Member Author

findepi commented Jan 4, 2021

squashed

@findepi findepi merged commit 6a9be6b into trinodb:master Jan 5, 2021
@findepi findepi deleted the findepi/simplify-docker-image-configuration-29b31e branch January 5, 2021 07:36
@findepi findepi added this to the 352 milestone Jan 5, 2021
@findepi findepi mentioned this pull request Jan 5, 2021
10 tasks
kemalty added a commit to kemalty/trino that referenced this pull request May 11, 2022
After the change in Trino file structures (trinodb#6497), this document was not updated. You can see that this document page is the same in both current and old versions:

https://trino.io/docs/current/connector/mysql.html

https://prestodb.io/docs/current/connector/mysql.html

Because the similarities in the old and new file structures, this document became confusing.  Before the catalogs were in `/usr/lib/trino/etc`. Now they are in `/etc/trino/`. This nuance resulted in missing catalogs in the new version.

See also: https://trinodb.slack.com/archives/CGB0QHWSW/p1652278657730469
@kemalty kemalty mentioned this pull request May 11, 2022
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.

None yet

3 participants