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

Remove obsolete compose version specifier in Example docker-compose.yaml #8606

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

SimonCW
Copy link
Contributor

@SimonCW SimonCW commented Oct 23, 2024

Description

Version specifier for docker compose is obsolete. See: https://docs.docker.com/reference/compose-file/version-and-name/#version-top-level-element-obsolete

Version

all

Checklist

  • By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and subject to the Developers Certificate of Origin.
    For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Copy link

Thank you for submitting your PR. The PR states are In progress (or Draft) -> Tech review -> Doc review -> Editorial review -> Merged.

Before you submit your PR for doc review, make sure the content is technically accurate. If you need help finding a tech reviewer, tag a maintainer.

When you're ready for doc review, tag the assignee of this PR. The doc reviewer may push edits to the PR directly or leave comments and editorial suggestions for you to address (let us know in a comment if you have a preference). The doc reviewer will arrange for an editorial review.

@kolchfa-aws
Copy link
Collaborator

@peterzhuamazon Could you review this PR?

@peterzhuamazon
Copy link
Member

peterzhuamazon commented Oct 23, 2024

Hi,

If we need to proceed on this, then more files need to be updated (might be more):
https://github.com/opensearch-project/project-website/blob/main/samples/1.x/docker-compose.yml
https://github.com/opensearch-project/project-website/blob/main/samples/docker-compose.yml
https://github.com/opensearch-project/opensearch-build/tree/main/docker/release/dockercomposefiles

Personally I dont think it is necessary as it is used in several places already.
At most it is sending a warning about the obsolete setting and will not impact the actual deployment.

Thanks.

@SimonCW
Copy link
Contributor Author

SimonCW commented Oct 24, 2024

Hi,

If we need to proceed on this, then more files need to be updated (might be more): https://github.com/opensearch-project/project-website/blob/main/samples/1.x/docker-compose.yml https://github.com/opensearch-project/project-website/blob/main/samples/docker-compose.yml https://github.com/opensearch-project/opensearch-build/tree/main/docker/release/dockercomposefiles

Personally I dont think it is necessary as it is used in several places already. At most it is sending a warning about the obsolete setting and will not impact the actual deployment.

Thanks.

True, it just gives a warning. I still think it can be confusing, especially for new / occasional docker compose users. For example, I searched the full 70 line docker-compose.yml expecting a problem in the meat of the OpenSearch config. Immediately before, I introduced a OPEN_SEARCH_VERSION environment variable to add to the confusion. But I appreciate that there are more places to fix for consistency.

If you're still interested, I'll als remove the other mentions and collect the PRs here for review. @peterzhuamazon

@epugh
Copy link
Collaborator

epugh commented Oct 24, 2024

I've been slowly migrating my various open source projects from docker-compose to docker compose and removing the versions. It's a pain, but also makes sure the project doesn't fall behind what Docker expects!

@peterzhuamazon
Copy link
Member

Hi,
If we need to proceed on this, then more files need to be updated (might be more): https://github.com/opensearch-project/project-website/blob/main/samples/1.x/docker-compose.yml https://github.com/opensearch-project/project-website/blob/main/samples/docker-compose.yml https://github.com/opensearch-project/opensearch-build/tree/main/docker/release/dockercomposefiles
Personally I dont think it is necessary as it is used in several places already. At most it is sending a warning about the obsolete setting and will not impact the actual deployment.
Thanks.

True, it just gives a warning. I still think it can be confusing, especially for new / occasional docker compose users. For example, I searched the full 70 line docker-compose.yml expecting a problem in the meat of the OpenSearch config. Immediately before, I introduced a OPEN_SEARCH_VERSION environment variable to add to the confusion. But I appreciate that there are more places to fix for consistency.

If you're still interested, I'll als remove the other mentions and collect the PRs here for review. @peterzhuamazon

Thanks @SimonCW 👍 ,

Yeah if you can send me the batch of PRs here I will get them reviewed before we merge this one, since this one basically change the documentation website.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants