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

Use version-specific documentation link in jvm.options file #76323

Merged
merged 3 commits into from
Aug 11, 2021

Conversation

mark-vieira
Copy link
Contributor

We use a hard-coded link to the "current" documentation on Java heap settings in our jvm.options file which may very well be out of date with the corresponding version of Elasticsearch the file is coming from.

Closes #65808

@mark-vieira mark-vieira added >non-issue :Delivery/Packaging RPM and deb packaging, tar and zip archives, shell and batch scripts v7.15.0 labels Aug 10, 2021
@elasticmachine elasticmachine added the Team:Delivery Meta label for Delivery team label Aug 10, 2021
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-delivery (Team:Delivery)

@mark-vieira
Copy link
Contributor Author

@DaveCTurner one thing that just occurred to me after creating this PR is that we might run into issues when folks upgrade, but keep their old jvm.options file (which I suspect is common). In that case we'll be linking to a potentially old version of the docs. There's no good way to solve both problems of linking to docs that are too new and linking to docs that are too old so I suspect we have to pick the worse of the two evils here. Thoughts?

@mark-vieira mark-vieira added the auto-backport Automatically create backport pull requests when merged label Aug 10, 2021
@DaveCTurner
Copy link
Contributor

Thanks for addressing this Mark.

I think it's better to link to the docs that correspond with the file as originally installed (i.e. what you've done here). It's also fairly common to install older versions afresh, and having a fresh install link to the wrong docs will cause more of a surprise than accumulating cruft through upgrades.

We should know that the version-specific link will continue to exist even if we move things around in newer versions, and the page does warn you that it's not the latest version at the top, with facilities to choose the same page for a different version, so I think folks will get to the right place in the end.

Copy link
Contributor

@DaveCTurner DaveCTurner left a comment

Choose a reason for hiding this comment

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

I figured if this didn't work (e.g. we spelled project.minor.version differently in the two places) then we wouldn't be able to build a distribution. But then I tried it and discovered that it didn't, it just left the mis-spelled @project.minor.vesrion@ literally in the file. Should we be failing the build on that?

Also I think we should make https://www.elastic.co/guide/en/elasticsearch/reference/current/jvm-options.html be version-specific too.

@breskeby breskeby self-requested a review August 11, 2021 07:40
Copy link
Contributor

@breskeby breskeby left a comment

Choose a reason for hiding this comment

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

lgtm. would be indeed great if we could fail the build if a token is not been replaced, but as we're using ReplaceToken filter I don't see how to do that in a straight forward fashion.

@DaveCTurner
Copy link
Contributor

DaveCTurner commented Aug 11, 2021

Acked, ok we'll take the risk of typos here then.

I still think we should fix the link to jvm-options.html too tho.

@mark-vieira mark-vieira merged commit 81616c2 into elastic:master Aug 11, 2021
@mark-vieira mark-vieira deleted the version_specific_doc_link branch August 11, 2021 21:31
@elasticsearchmachine
Copy link
Collaborator

💔 Backport failed

Status Branch Result
7.x Commit could not be cherrypicked due to conflicts

To backport manually run backport --upstream elastic/elasticsearch --pr 76323

mark-vieira added a commit to mark-vieira/elasticsearch that referenced this pull request Aug 11, 2021
…76323)

# Conflicts:
#	distribution/src/config/jvm.options
elasticsearchmachine pushed a commit that referenced this pull request Aug 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Automatically create backport pull requests when merged :Delivery/Packaging RPM and deb packaging, tar and zip archives, shell and batch scripts >non-issue Team:Delivery Meta label for Delivery team v7.15.0 v8.0.0-alpha2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JVM options file contains links to current docs rather than a specific version
6 participants