Skip to content

Change - {version} by ${VERSION} #1851

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

Closed
wants to merge 2 commits into from
Closed

Conversation

rauhmaru
Copy link

Adjust in config files, changing {version} by ${VERSION}

Adjust in config files, changing {version} by ${VERSION}
@cla-checker-service
Copy link

cla-checker-service bot commented Oct 19, 2021

💚 CLA has been signed

@gtback
Copy link
Member

gtback commented Feb 23, 2022

cla/check

@gtback
Copy link
Member

gtback commented Feb 23, 2022

Thanks @rauhmaru ! I'm not a Docker compose expert so I'll leave this to @bytebilly or @lockewritesdocs to review. It looks like in #1906 this file got changed to having CRLF line endings. I'm not sure if this was intentional, but it took a bit of work to resolve the conflict of getting the latest main merged into this.

@gtback gtback added backport-7.17 Automated backport with mergify backport-8.0 Automated backport with mergify backport-8.1 Automated backport with mergify and removed backport-8.1 Automated backport with mergify backport-8.0 Automated backport with mergify backport-7.17 Automated backport with mergify labels Feb 23, 2022
Copy link
Contributor

@bytebilly bytebilly left a comment

Choose a reason for hiding this comment

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

Uh, this change doesn't work as it should be ${STACK_VERSION} to correctly load the variable from the .env file.
This file should be exactly the same as https://github.com/elastic/elasticsearch/tree/master/docs/reference/setup/install/docker-compose.yml.

Is there a way to ensure that we have just one version of it? This will prevent future desync.

@gtback
Copy link
Member

gtback commented Feb 24, 2022

I went looking into how this file was used, and it seems like it's substituting AsciiDoc variables in here rather than variables from the .env file. That's why it shows "8.0.0" here.

In terms of single-sourcing the file, we don't currently depend on the elasticsearch repo in the Getting Started guide, but we could add that around here. It's probably best to let Adam take a look when he's back.

@bytebilly
Copy link
Contributor

I went looking into how this file was used, and it seems like it's substituting AsciiDoc variables in here rather than variables from the .env file. That's why it shows "8.0.0" here.

Yes, that's the problem that I tried to fix in elastic/elasticsearch#83823, but I haven't realized that docs use a different source file.

I'd suggest to start fixing the docs in this PR (by just copying the right file in this repo), and then we can iterate and see if we can have a ssot.

@mergify
Copy link
Contributor

mergify bot commented Feb 24, 2022

This pull request is now in conflict. Could you fix it @rauhmaru? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch <remote-repo>
git checkout -b patch-1 <remote-repo>/patch-1
git merge <remote-repo>/main
git push <remote-repo> patch-1

@mergify
Copy link
Contributor

mergify bot commented Feb 24, 2022

This pull request does not have a backport label. Could you fix it @rauhmaru? 🙏
To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

  • backport-/d./d is the label to automatically backport to the /d./d branch. /d is the digit
    NOTE: backport-skip has been added to this pull request.

@mergify mergify bot added the backport-skip Skip automated backport with mergify label Feb 24, 2022
@lcawl
Copy link
Contributor

lcawl commented Feb 25, 2022

I have updated the Getting Started Guide to pull the docker-compose.yml .env files from the elasticsearch repo in #2069, so I believe this PR can be closed. Thank you for drawing attention to this discrepancy!

@lcawl lcawl closed this Feb 25, 2022
@gtback
Copy link
Member

gtback commented Feb 25, 2022

Thanks for bringing this to our attention, @rauhmaru ! Even though we ended up not merging this, it prompted us to improve this documentation, and we are very grateful for that! 🙏🏻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-skip Skip automated backport with mergify
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants