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 conditions for each platform #8811

Merged
merged 3 commits into from
Nov 1, 2018

Conversation

dedemorton
Copy link
Contributor

Summary of changes:

  • Added attributes to use for conditionally rendering commands based on the platforms supported by a Beat (this is required by Journalbeat).
  • Created a separate example for each OS. We could probably continue combining deb and rpm in a single command example, but separating them gives us maximum flexibility for conditionally rendering examples in the future.
  • Removed allplatforms and changed the files to use win-only to render content that's only relevant when windows is the only supported platform (currently true for Winlogbeat).
  • Added attribute resets where the standalone attribute is used (resetting attributes when you no longer want them to be in scope is a best practice).
  • Added attribute names to endif statements to make it easier to read the asciidoc source when there are nested conditions.

Open issues (to be addressed in future PRs):

  • I still need to add linux commands. I'll do that in another PR.
  • The docker commands are a bit of a mess (inconsistent coverage). It's been suggested that we remove the docker commands from the GS and point users to the topic about running on docker. I might do this, but wanted the focus of this PR to be conditional settings.

@ph
Copy link
Contributor

ph commented Oct 31, 2018

@dedemorton I had a quick look this morning, I think the changes look OK. But I see that we are repeating a lot of the same information depending if we are in a deb/rpm/macos/ doc. Could we change the conditions to include more than one platform in the ifdef statement and reduce the duplication?


ifeval::["{requires-sudo}"=="yes"]

include::../../libbeat/docs/shared-note-sudo.asciidoc[]

endif::[]

*deb and rpm:*
ifdef::deb_os[]
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can do ifdef::deb_os,rpm_os[] example link

Copy link
Contributor Author

@dedemorton dedemorton Oct 31, 2018

Choose a reason for hiding this comment

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

@ph I kind of grappled with this, too, because I don't like the repetition. I wanted to make sure that the conditional coding is granular enough to deal with situations where we might want to tag something for one OS but not the other. However, if there are two OS's that will always be supported together, then I can combine the commands. I think deb/rpm might be in this category, but I think Mac OS needs to be separate. WDYT? Linux + MacOS won't work together because Journalbeat (I believe) is linux-only, right? If you can help me identify the correct granularity for the conditions, I'll remove as much duplication as I can. My dream is to have a widget to switch between OS commands in the docs so things don't look so repetitive. (note that I haven't added the linux commands yet but I plan to)

Copy link
Contributor

Choose a reason for hiding this comment

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

@dedemorton I think deb/rpm should be almost always the same. But yes you are right journalbeat is not on windows and not on mac.

Copy link
Contributor

@ph ph Oct 31, 2018

Choose a reason for hiding this comment

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

We could have a unix flavor? Looking a the doc, all of the mac, deb, rpm are similar the same.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looking the deb, rpm and macos commands they are pretty much the same, except that on mac you have ./ prefix to make sure we call the binaries in the current directory.

libbeat/docs/shared-template-load.asciidoc Outdated Show resolved Hide resolved

ifeval::["{requires-sudo}"=="yes"]

include::../../libbeat/docs/shared-note-sudo.asciidoc[]

endif::[]

*deb and rpm:*
ifdef::deb_os[]
Copy link
Contributor

Choose a reason for hiding this comment

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

Looking the deb, rpm and macos commands they are pretty much the same, except that on mac you have ./ prefix to make sure we call the binaries in the current directory.

@dedemorton
Copy link
Contributor Author

dedemorton commented Oct 31, 2018

@ph OK, I've merged the rpm/deb commands into single examples. For the commands in shared files (where the conditional coding is necessary), I used OR (deb_os,rpm_os) instead AND (deb_os+rpm_os). This will be weird if we ever have a Beat that supports either deb or rpm, but not both, but it avoids the situation where the command gets dropped completely if someone defines one attribute (like deb_os) but not the other.

@dedemorton dedemorton mentioned this pull request Nov 1, 2018
8 tasks
@dedemorton dedemorton added the needs_backport PR is waiting to be backported to other branches. label Nov 1, 2018
@dedemorton dedemorton merged commit e1f8a19 into elastic:master Nov 1, 2018
dedemorton added a commit to dedemorton/beats that referenced this pull request Nov 9, 2018
* Add conditions for each platform

* Add conditions to directory layout topic

* Combine conditions for deb and rpm
dedemorton added a commit to dedemorton/beats that referenced this pull request Nov 9, 2018
* Add conditions for each platform

* Add conditions to directory layout topic

* Combine conditions for deb and rpm
@dedemorton dedemorton removed the needs_backport PR is waiting to be backported to other branches. label Nov 9, 2018
dedemorton added a commit that referenced this pull request Nov 12, 2018
* Add conditions for each platform (#8811)

* Add conditions for each platform

* Add conditions to directory layout topic

* Combine conditions for deb and rpm

* Update Journalbeat docs (#8864)

* More updates for journalbeat

* Remove unwanted comments and extra files

* Add changes from review

* Changes from second round of reviews

* Remove backoff options

* Remove questions for reviewers

* Change file to journal

* Change default  max_backoff in docs to 20s

* More fixes from review

* Revert "Remove backoff options"

This reverts commit d57e4b3.

* Add global options back

* Initialize docs for functionbeat (#8744)

* Initialize docs

* Run make update

* In progress fixes from review

* More changes from the review and testing

* Add fixes from the review

* Fixed download path

* Remove unused dashboard image placeholders

* Add missing options and remove boiler plate text

* Run make update

* Remove broken link
dedemorton added a commit that referenced this pull request Nov 12, 2018
* Add conditions for each platform (#8811)

* Add conditions for each platform

* Add conditions to directory layout topic

* Combine conditions for deb and rpm

* Update Journalbeat docs (#8864)

* More updates for journalbeat

* Remove unwanted comments and extra files

* Add changes from review

* Changes from second round of reviews

* Remove backoff options

* Remove questions for reviewers

* Change file to journal

* Change default  max_backoff in docs to 20s

* More fixes from review

* Revert "Remove backoff options"

This reverts commit d57e4b3.

* Add global options back

* Initialize docs for functionbeat (#8744)

* Initialize docs

* Run make update

* In progress fixes from review

* More changes from the review and testing

* Add fixes from the review

* Fixed download path

* Remove unused dashboard image placeholders

* Add missing options and remove boiler plate text

* Run make update

* Remove broken link
DStape pushed a commit to DStape/beats that referenced this pull request Aug 20, 2019
* Add conditions for each platform

* Add conditions to directory layout topic

* Combine conditions for deb and rpm
DStape pushed a commit to DStape/beats that referenced this pull request Aug 20, 2019
…tic#8744) (elastic#9024)

* Add conditions for each platform (elastic#8811)

* Add conditions for each platform

* Add conditions to directory layout topic

* Combine conditions for deb and rpm

* Update Journalbeat docs (elastic#8864)

* More updates for journalbeat

* Remove unwanted comments and extra files

* Add changes from review

* Changes from second round of reviews

* Remove backoff options

* Remove questions for reviewers

* Change file to journal

* Change default  max_backoff in docs to 20s

* More fixes from review

* Revert "Remove backoff options"

This reverts commit d57e4b3.

* Add global options back

* Initialize docs for functionbeat (elastic#8744)

* Initialize docs

* Run make update

* In progress fixes from review

* More changes from the review and testing

* Add fixes from the review

* Fixed download path

* Remove unused dashboard image placeholders

* Add missing options and remove boiler plate text

* Run make update

* Remove broken link
leweafan pushed a commit to leweafan/beats that referenced this pull request Apr 28, 2023
…tic#8744)  (elastic#9025)

* Add conditions for each platform (elastic#8811)

* Add conditions for each platform

* Add conditions to directory layout topic

* Combine conditions for deb and rpm

* Update Journalbeat docs (elastic#8864)

* More updates for journalbeat

* Remove unwanted comments and extra files

* Add changes from review

* Changes from second round of reviews

* Remove backoff options

* Remove questions for reviewers

* Change file to journal

* Change default  max_backoff in docs to 20s

* More fixes from review

* Revert "Remove backoff options"

This reverts commit d57e4b3.

* Add global options back

* Initialize docs for functionbeat (elastic#8744)

* Initialize docs

* Run make update

* In progress fixes from review

* More changes from the review and testing

* Add fixes from the review

* Fixed download path

* Remove unused dashboard image placeholders

* Add missing options and remove boiler plate text

* Run make update

* Remove broken link
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.

2 participants