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

Add missing info about Homebrew installation #28902

Merged
merged 8 commits into from
Jun 20, 2018
Merged

Add missing info about Homebrew installation #28902

merged 8 commits into from
Jun 20, 2018

Conversation

jrpool
Copy link
Contributor

@jrpool jrpool commented Mar 5, 2018

A better fix is probably to make the above change and also to put the “Successfully running node” section into a different file and refer users to it as the next file at the end of each installation section. It is currently difficult to ascertain that that section pertains to all of the installation platforms and methods, not only to the MSI Windows one.

  • Have you signed the contributor license agreement?
  • Have you followed the contributor guidelines?
  • If submitting code, have you built your formula locally prior to submission with gradle check?
  • If submitting code, is your pull request against master? Unless there is a good reason otherwise, we prefer pull requests against master and will backport as needed.
  • If submitting code, have you checked that your submission is for an OS that we support?
  • If you are submitting this code for a class then read our policy for that.

A better fix is probably to make the above change and also to put the “Successfully running node” section into a different file and refer users to it as the next file at the end of each installation section. It is currently difficult to ascertain that that section pertains to all of the installation platforms and methods, not only to the MSI Windows one.
@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

1 similar comment
@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@javanna javanna added :Delivery/Packaging RPM and deb packaging, tar and zip archives, shell and batch scripts review labels Mar 6, 2018
@javanna
Copy link
Member

javanna commented Mar 6, 2018

cc @elastic/es-core-infra

@colings86 colings86 added the >docs General docs changes label Apr 24, 2018
Copy link
Member

@cbuescher cbuescher left a comment

Choose a reason for hiding this comment

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

@jrpool Hi, thanks for the suggestion and sorry for getting to this only just now. I left a comment with a suggestion of how to enhance the change a bit, but we don't have to. Let me know if you agree and want to make the suggested change, I can merge it in then.

@@ -158,6 +158,8 @@ On macOS, Elasticsearch can also be installed via https://brew.sh[Homebrew]:
brew install elasticsearch
--------------------------------------------------

If installation succeeds, Homebrew will finish by saying that you can start Elasticsearch by entering `elasticsearch`. Do that now. The expected response is described below, under `Successfully running node`.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe you could add an anchor like [[successfully-running-node]] to the section in question and then reference it with an internal cross references <<....>>? I think that would make navigating to the next step even easier.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you, @cbuescher. I lost track of your suggestion. Now I have implemented it, after a long delay.

@cbuescher cbuescher self-assigned this May 25, 2018
@cbuescher
Copy link
Member

@jrpool Hi, just checking in with you to see if you are still interested in adding this PR to the documentation. I think its a good change and the comment I left is purely optional, but I would like to get your feedback and maybe see if we can improve with the small suggested change before merging this.
Let me know if you want to continue this PR, otherwise I'd also consider closing it, but I think it would be a nice addition.

@cbuescher
Copy link
Member

Hi @jrpool, great you want to add the internal link as an improvement. Unfortunately the link still isn't working for me. I think you need to add an anchor tag as a target to the section you link to. I will mark the place I think it needs to go as a comment. If you are interested I can also point you to the steps you can take to test building the markdown of a documentation page locally, however if you are not planning to do this a lot its probably a bit too much work. I can also add the change on top of your PR if you like, but maybe you prefer to do that yourself. Just let me know.

@@ -158,6 +158,8 @@ On macOS, Elasticsearch can also be installed via https://brew.sh[Homebrew]:
brew install elasticsearch
--------------------------------------------------

If installation succeeds, Homebrew will finish by saying that you can start Elasticsearch by entering `elasticsearch`. Do that now. The expected response is described below, under <<successfully-running-node>>.
Copy link
Member

Choose a reason for hiding this comment

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

It appears I cannot easily make comments in sections that were not changed, so I'm adding it here. I think you need an anchor [[successfully-running-node]] later above the section title you are linking to. I added it locally at line 221 (between the [float] marker and the section title). Another small thing if you are editing this file another time anyway: we try to keep the line length a bit shorter (although this is not consistently applied throughout the docs, as demonstrated just a few lines below), so if you don't mind I think this could use a line break maybe after the first sentence. Not a necessary change though.

Several others exist in this file that could be split for consistency.
@jrpool
Copy link
Contributor Author

jrpool commented Jun 19, 2018

I have split the long line, @cbuescher, but I suggest we discuss the fact that the link works in my environment and not in yours.

The HTML source for the link, as I see in in the Github interface, is:

“The expected response is described below, under
<a href="#successfully-running-node">Successfully running node</a>.”

The destination heading is:

<h3 id="user-content-successfully-running-node">
<a id="user-content-successfully-running-node"
class="anchor" aria-hidden="true" href="#successfully-running-node">
<svg class="octicon octicon-link" viewBox="0 0 16 16" version="1.1" width="16" height="16" aria-hidden="true">
<path fill-rule="evenodd" d="M4 9h1v1H4c-1.5 0-3-1.69-3-3.5S2.55 3 4 3h4c1.45 0 3 1.69 3 3.5 0 1.41-.91 2.72-2 3.25V8.59c.58-.45 1-1.27 1-2.09C10 5.22 8.98 4 8 4H4c-.98 0-2 1.22-2 2.5S3 9 4 9zm9-3h-1v1h1c1 0 2 1.22 2 2.5S13.98 12 13 12H9c-.98 0-2-1.22-2-2.5 0-.83.42-1.64 1-2.09V6.25c-1.09.53-2 1.84-2 3.25C6 11.31 7.55 13 9 13h4c1.45 0 3-1.69 3-3.5S14.5 6 13 6z"></path>
</svg>
</a>
Successfully running node
</h3>

I understand that Github Flavored Markdown imports CSS and/or JS that creates an unprefixed link hidden except on hover, and in my environment the link goes there. What user agent are you using?

@cbuescher
Copy link
Member

I suspect you are referring to https://github.com/jrpool/elasticsearch/blob/6194bb18c920c47ff05258d29becfe4f2b50a06f/docs/reference/getting-started.asciidoc? When I mean the link doesn't work I refer to building our documentation that is pushed to the webpage as described in https://github.com/cbuescher/docs/blob/master/README.asciidoc.
I think the scripts we are using take the Markdown and combine them in A bigger document. When I run our documentation build scripts locally I see a missing link. I see three ways to solve this:

  • I can walk you through the steps needed to build the docs with that script
  • You can simply add the anchor that I mentioned where it should go and I check its working
  • I can push the necessary changes to this branch myself and merge it with your change

Let me know whatever works for you.

@jrpool
Copy link
Contributor Author

jrpool commented Jun 19, 2018

Thanks for explaining this issue. I don’t expect to be working with ElasticSearch much in the near future, so I have opted for one of the easier alternatives. If it isn’t entirely correct, please feel free to add an additional fix.

@cbuescher
Copy link
Member

Great, thanks for pushing the commit. I will kick off some final tests before merging.
@elasticmachine test this please

@cbuescher
Copy link
Member

@elasticmachine test this please

@cbuescher cbuescher merged commit 1f8e3a4 into elastic:6.2 Jun 20, 2018
cbuescher pushed a commit that referenced this pull request Jun 20, 2018
Adding a note about proceeding after a successful homebrew installation.
cbuescher pushed a commit that referenced this pull request Jun 20, 2018
Adding a note about proceeding after a successful homebrew installation.
cbuescher pushed a commit that referenced this pull request Jun 20, 2018
Adding a note about proceeding after a successful homebrew installation.
@cbuescher
Copy link
Member

@jrpool thanks again for your time. I merged the change to the 6.2 documentation and newer versions.

dnhatn added a commit that referenced this pull request Jun 20, 2018
* 6.x:
  [DOCS] Omit shard failures assertion for incompatible responses  (#31430)
  [DOCS] Move licensing APIs to docs (#31445)
  backport of: add is-write-index flag to aliases (#30942) (#31412)
  backport of: Add rollover-creation-date setting to rolled over index (#31144) (#31413)
  [Docs] Extend Homebrew installation instructions (#28902)
  [Docs] Mention ip_range datatypes on ip type page (#31416)
  Multiplexing token filter (#31208)
  Fix use of time zone in date_histogram rewrite (#31407)
  Revert "Mute DefaultShardsIT#testDefaultShards test"
  [DOCS] Fixes code snippet testing for machine learning (#31189)
  Security: fix joining cluster with production license (#31341)
  [DOCS] Updated version in Info API example
  [DOCS] Moves the info API to docs (#31121)
  Revert "Increasing skip version for failing test on 6.x"
  Preserve response headers on cluster update task (#31421)
  [DOCS] Add code snippet testing for more ML APIs (#31404)
  Docs: Advice for reindexing many indices (#31279)
dnhatn added a commit that referenced this pull request Jun 20, 2018
* master:
  [DOCS] Omit shard failures assertion for incompatible responses  (#31430)
  [DOCS] Move licensing APIs to docs (#31445)
  Add Delete Snapshot High Level REST API
  Remove QueryCachingPolicy#ALWAYS_CACHE (#31451)
  [Docs] Extend Homebrew installation instructions (#28902)
  Choose JVM options ergonomically
  [Docs] Mention ip_range datatypes on ip type page (#31416)
  Multiplexing token filter (#31208)
  Fix use of time zone in date_histogram rewrite (#31407)
  Core: Remove index name resolver from base TransportAction (#31002)
  [DOCS] Fixes code snippet testing for machine learning (#31189)
  [DOCS] Removed  and  params from MLT. Closes #28128 (#31370)
  Security: fix joining cluster with production license (#31341)
  Unify http channels and exception handling (#31379)
  [DOCS] Moves the info API to docs (#31121)
  Preserve response headers on cluster update task (#31421)
  [DOCS] Add code snippet testing for more ML APIs (#31404)
  Do not preallocate bytes for channel buffer (#31400)
  Docs: Advice for reindexing many indices (#31279)
  Mute HttpExporterTests#testHttpExporterShutdown test Tracked by #31433
  Docs: Add note about removing prepareExecute from the java client (#31401)
  Make release notes ignore the `>test-failure` label. (#31309)
@mark-vieira mark-vieira added the Team:Delivery Meta label for Delivery team label Nov 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Delivery/Packaging RPM and deb packaging, tar and zip archives, shell and batch scripts >docs General docs changes Team:Delivery Meta label for Delivery team v6.2.5 v6.3.1 v6.4.0 v7.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants