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 more detail topic about downloading docker images #7615

Merged
merged 1 commit into from
Jul 18, 2018

Conversation

dedemorton
Copy link
Contributor

I wasn't completely satisfied with the language that we added in elastic/logstash#9831, so I've made some changes. The original text was a bit verbose, overly passive, and ambiguous in places. If you like the changes, we can get them into the other repos.

  • Removed the passive voice from the second paragraph and tightened up the language just a little.
  • Removed the following sentence because I don't think it adds anything new: "For example, the Docker image can be retrieved with the following command." If you think this is necessary, I can add it back in...but IMO, it is redundant.
  • Broke the sentence that begins with "Alternatively, you can download..." into two sentences. The original sentence has a dangling modifier. Grammatically, it's saying that the Apache license is available from the docker page. Passed it by Gail, and she agreed that it was hard to parse.

I've also added the license content that wasn't in the Beats topic originally. Let me know if it's not completely accurate for Beats.

@dedemorton dedemorton added docs review needs_backport PR is waiting to be backported to other branches. labels Jul 17, 2018
@tbragin
Copy link
Contributor

tbragin commented Jul 17, 2018

Thanks for taking a pass @dedemorton! LGTM.

As you suggest, it'd be great to get these same changes into the other places in the stack we are listing docker pull commands.

Copy link
Contributor

@tsg tsg left a comment

Choose a reason for hiding this comment

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

LGTM, thank you @dedemorton

@ruflin ruflin merged commit bade413 into elastic:master Jul 18, 2018
@dedemorton dedemorton deleted the add_docker_image_info branch July 20, 2018 23:53
dedemorton added a commit to dedemorton/beats that referenced this pull request Jul 20, 2018
@dedemorton dedemorton removed the needs_backport PR is waiting to be backported to other branches. label Jul 21, 2018
leweafan pushed a commit to leweafan/beats that referenced this pull request Apr 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants